-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #49 and Issue #50 : Offheap Resource #51
Conversation
c91ddb8
to
9fcccdc
Compare
There is the usual concern I have with units: saying "MB" when we actually interpret it as "MiB", etc. Might want some doc at the top of OffHeapResource to mention that the service merely does accounting and the caller is expected to actually do the allocation/free. The API makes that pretty clear, though, so it might not matter. Otherwise, this sounds like exactly what you described. The lack of a "default" pool is good (the named resource really should be explicit). I also like that this doesn't try to position itself as a generic service but makes it clear that this is only for off-heap. I am happy with this. |
} | ||
} | ||
|
||
public boolean allocate(long size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just call this method alloc
? I usuallu don't like shortened names but alloc/free is a common pattern so I personally prefer to use well-known names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except it's not actually "alloc" since it doesn't really allocate. I need to add some javadoc here anyway... so I can make the equivalence more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to differentiate this service from the traditional alloc/free -which I think makes sense- shouldn't you use other terms in the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.. switching to reserve and release.
any plans for validating against available/physical size? |
I could easily add this... will look in to doing this and warning about it. I'd rather not hard fail on this though since this service doesn't actually allocate anything at all... so it seems a bit too heavy handed to fail if we're short on physical memory. |
9fcccdc
to
57eb265
Compare
57eb265
to
b2c3f22
Compare
I would be in favour of failing fast if we know from the start the memory is not available. At least certainly if the total physical memory is less than configured. And if the free memory is less, I would still fail as well ... |
I have created #53 for tracking the overcommit behavior. Until we have something closer to a complete system I don't want to make any decisions here, since I believe this has to be approached in a holistic manner. |
rename methods for better lisibility
I'm most interested in people's review of the XML configuration I'm proposing here. The implementation is a little rougher around the edges... and will likely get cleaned up as I work with/on it.
@ljacomet @cljohnso @myronkscott @jd0-sag you guys seem like a good shortlist for reviewers.