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

ChunkedAssociativeLongArray re-use expired nodes #1145

Merged
merged 2 commits into from
Jun 23, 2017
Merged

ChunkedAssociativeLongArray re-use expired nodes #1145

merged 2 commits into from
Jun 23, 2017

Conversation

storozhukBM
Copy link
Contributor

This change gives ChunkedAssociativeLongArray ability to store chunks that are expired and removed from main data structure.
Then instead of allocating new Chunk immediately we are trying to poll one from this cache.
So if you have constant or slowly changing load ChunkedAssociativeLongArray will never throw away old chunks or allocate new ones which makes this data structure almost garbage free. Also chunks stored in cache as SoftReferences so this memory can actually be freed if nesesary.


Numbers:
In ReservoirBenchmark: updates are 5.9x faster and ~370x less time spent in GC than SlidingTimeWindowReservoir.
In SlidingTimeWindowReservoirsBenchmark: updates are 3.2x faster, reads - 2.7x faster, 37x less time spent in GC. (here we see small difference because most time we spent in cleaning snapshots)


Graphs:

Comparison of implementations under constant load [duration 1h]
constant load comparison


Comparison of implementations under changing load [duration 1h]
changing load comparison

@storozhukBM
Copy link
Contributor Author

Hi @arteam,
It would be nice if you have a chance to look at this PR before release.

@storozhukBM
Copy link
Contributor Author

storozhukBM commented Jun 19, 2017

Here is great illustration of chunks reuse 😄 . link

@storozhukBM
Copy link
Contributor Author

storozhukBM commented Jun 21, 2017

Hi @ryantenney, thank you for merge.
🎉

@ryantenney
Copy link
Contributor

I didn't merge, but only because I didn't want to screw up the release @arteam was planning! Just waiting for him to review and merge!

@@ -22,11 +43,34 @@
this.defaultChunkSize = chunkSize;
}

private Chunk allocateChunk() {
SoftReference<Chunk> chunkRef = chunksCache.pollLast();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about simplifying this method to:

while (true) {
  final SoftReference<Chunk> chunkRef = chunksCache.pollLast();
  if (chunkRef == null) {
    return new Chunk(defaultChunkSize);
  }
  final Chunk chunk = chunkRef.get();
  if (chunk != null) {
    chunk.cursor = 0;
    chunk.startIndex = 0;
    chunk.chunkSize = chunk.keys.length;
    return chunk;
  }
}


private final int defaultChunkSize;
/*
We use this ArrayDeque as cache to store chunks that are expired and removed from main data structure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use multi-line comments with an asterix on every line. We are trying to follow the Google Java Style Guide. See section about comments.

@storozhukBM
Copy link
Contributor Author

Hi @arteam,
Thank you for review. All comments fixed.

@arteam arteam merged commit 2d43206 into dropwizard:3.2-development Jun 23, 2017
@arteam arteam added this to the 3.2.3 milestone Jun 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants