Skip to content
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

Merged
merged 4 commits into from
Mar 23, 2016

Conversation

chrisdennis
Copy link
Member

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.

@jd0-sag
Copy link
Contributor

jd0-sag commented Mar 19, 2016

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) {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

@ramsai1729
Copy link
Contributor

any plans for validating against available/physical size?

@chrisdennis
Copy link
Member Author

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.

@ljacomet
Copy link
Member

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 ...

@chrisdennis
Copy link
Member Author

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.

@ljacomet ljacomet merged commit 3e5ca83 into Terracotta-OSS:master Mar 23, 2016
anthonydahanne added a commit to anthonydahanne/terracotta-platform that referenced this pull request Jun 28, 2016
@chrisdennis chrisdennis deleted the offheap-resource branch August 9, 2017 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants