Add tracking of allocated arrays. #5264
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Neat. |
|
cool stuff @jpountz |
| @@ -39,16 +38,17 @@ | |||
| private final BytesRef spare; | |||
|
|
|||
| // Constructor with configurable capacity and default maximum load factor. | |||
| public BytesRefHash(long capacity, PageCacheRecycler recycler) { | |||
| this(capacity, DEFAULT_MAX_LOAD_FACTOR, recycler); | |||
| public BytesRefHash(long capacity, BigArrays bigArrays) { | |||
There was a problem hiding this comment.
Oh, yeah, cool. I can totally use that.
On Wed, Feb 26, 2014 at 3:24 PM, Simon Willnauer
notifications@github.comwrote:
In
src/main/java/org/elasticsearch/search/aggregations/bucket/BytesRefHash.java:@@ -39,16 +38,17 @@
private final BytesRef spare;// Constructor with configurable capacity and default maximum load factor.
- public BytesRefHash(long capacity, PageCacheRecycler recycler) {
this(capacity, DEFAULT_MAX_LOAD_FACTOR, recycler);- public BytesRefHash(long capacity, BigArrays bigArrays) {
I really like this!
Reply to this email directly or view it on GitHubhttps://github.com//pull/5264/files#r10097411
.
|
I left some comments but in general this looks awesome |
The BigArrays utility class is useful to generate arrays of various sizes: when small, arrays will be allocated directly on the heap while larger arrays are going to be paged and to recycle pages through PageCacheRecycler. We already have tracking for pages but this is not triggered very often since it only happens on large amounts of data while our tests work on small amounts of data in order to be fast. Tracking arrays directly helps make sure that we never forget to release them. This pull request also improves testing by: - putting random content in the arrays upon release: this makes sure that consumers don't use these arrays anymore when they are released as their content may be subject to use for another purpose since pages are recycled - putting random content in the arrays upon creation and resize when `clearOnResize` is `false`. The major difference with `master` is that the `BigArrays` class is now instanciable, injected via Guice and usually available through the `SearchContext`. This way, it can be mocked for tests.
|
Thanks for the feedback, @s1monw. I rebased in order to get @martijnvg 's changes around p/c field data and added a new commit. |
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The BigArrays utility class is useful to generate arrays of various sizes: when
small, arrays will be allocated directly on the heap while larger arrays are
going to be paged and to recycle pages through PageCacheRecycler. We already
have tracking for pages but this is not triggered very often since it only
happens on large amounts of data while our tests work on small amounts of data
in order to be fast.
Tracking arrays directly helps make sure that we never forget to release them.
This pull request also improves testing by:
consumers don't use these arrays anymore when they are released as their
content may be subject to use for another purpose since pages are recycled
clearOnResizeisfalse.The major difference with
masteris that theBigArraysclass is nowinstanciable, injected via Guice and usually available through the
SearchContext. This way, it can be mocked for tests.