Reduce newSeriesChunkRefsSet() introducing a memory pool #3666
Conversation
There was a problem hiding this comment.
pooling LGTM, i think it's missing one .reset
copying my comment from a private convo
I have a concern around forgetting to release a set. Right now whoever uses newSeriesChunkRefsSet needs to be aware of who is going to later release this set. It’s difficult to catch when we don’t release
i have two ideas but you’ll tell me if they are not overengineering or worrying too much. maybe easier to explain on a call. my ideas:
- use the iterator approach to release seriesChunkRefsSet back into the pool. We know which iterators are supposed to use newSeriesChunkRefsSet . Put an iterator before them, which does seriesChunkRefsSet.reset on the previous set every time it receives a Next call (every time after the first). This way we keep the logic of when to release something and when to allocate one slightly less coupled. This should make it easier to reason when something should be freed.
- have a per-request pool of these batches that we release after the request. Very similar to how pool.BatchBytes works. And instead of calling newSeriesChunkRefsSet directly, you use this pool to get a new set. This way we make sure we don’t leak from the pool between requests. It’s also harder to make a new allocation, because you need the pool, can’t just call a local function.
pracucci
force-pushed
the
reduce-newSeriesChunkRefsSet-allocations
branch
2 times, most recently
from
0127282
to
a4f3f81
Compare
Fixed the optimization for the case series are not duplicated among sets Added TestMergedSeriesChunkRefsSet_Concurrency Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
reduce-newSeriesChunkRefsSet-allocations
branch
from
a4f3f81
to
a5ec2c2
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
I agree it's hard, but I've introduced some assertions in the new tests to detect it.
I tried this approach, but ended up not being useable in every place. Try to take another look at the final version of the code. I personally think it's easier to follow than if the release is done through a wrapping iterator and the |
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
| // release the internal series slice to a memory pool. This function call has no effect | ||
| // if seriesChunkRefsSet was created to be not releasable. | ||
| // | ||
| // This function is not idempotent. Calling it twice would introduce subtle bugs. |
There was a problem hiding this comment.
Note to reviewers: since seriesChunkRefsSet is passed around as a copy, I can't set b.series = nil to make this function idempotent. I will try to pass this around as a pointer in a follow up PR, to see what's the impact on performance and code changes, and then we can take an informed decision.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
* Reduce newSeriesChunkRefsSet() introducing a memory pool Fixed the optimization for the case series are not duplicated among sets Added TestMergedSeriesChunkRefsSet_Concurrency Signed-off-by: Marco Pracucci <marco@pracucci.com> * Added TestBucket_Series_Concurrency Signed-off-by: Marco Pracucci <marco@pracucci.com> * Added assertions on seriesChunkRefsSetPool balance Signed-off-by: Marco Pracucci <marco@pracucci.com> * Fixed BenchmarkBucket_Series_WithSkipChunks Signed-off-by: Marco Pracucci <marco@pracucci.com> * Removed TODO Signed-off-by: Marco Pracucci <marco@pracucci.com> * Delete spurious file Signed-off-by: Marco Pracucci <marco@pracucci.com> * Address review feedback Signed-off-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
What this PR does
In this PR I'm proposing to reduce
newSeriesChunkRefsSet()introducing a memory pool. The contract proposed in this PR is:seriesChunkRefsSetIterator.At()is responsible torelease()aseriesChunkRefsSetonce not used anymore.seriesChunkRefsSetcan be declared non releseable when created. If so, therelease()has no effect. This is useful, for example, in tests where the set could be retained and reused.release()I added a comment to explain why it's safe (at least, why I think it is...).I've added a couple of new tests with concurrency. These tests also ensure all slices requested from pool are also released back (assert on balance = 0). I've tried to manually introduce a bug, releasing the same slice twice to the pool in different places of the code, and the new concurrency tests failed in all cases.
Benchmark results:
I've run 3 benchmarks:
Bucket_SeriesBucket_Series_WithSkipChunksMergedSeriesChunkRefsSetIterators(new)Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]