MergeIterator: allocate less memory at first #4341
Conversation
The `through` time is supposed to be the last time in the chunk, and having it one step higher was throwing off other tests and benchmarks. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
At 15-second scrape intervals a chunk covers 30 minutes, so 1,000 chunks is about three weeks, a highly un-representative test. Instant queries, such as those done by the ruler, will only fetch one chunk from each ingester. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
We were allocating 24x the number of streams of batches, where each batch holds up to 12 samples. By allowing `c.batches` to reallocate when needed, we avoid the need to pre-allocate enough memory for all possible scenarios. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham
force-pushed
the
tune-merge-iterator
branch
from
6119b68
to
b32d886
Compare
| @@ -112,8 +112,7 @@ func (c *mergeIterator) buildNextBatch(size int) bool { | |||
| for len(c.h) > 0 && (len(c.batches) == 0 || c.nextBatchEndTime() >= c.h[0].AtTime()) { | |||
| c.nextBatchBuf[0] = c.h[0].Batch() | |||
| c.batchesBuf = mergeStreams(c.batches, c.nextBatchBuf[:], c.batchesBuf, size) | |||
| copy(c.batches[:len(c.batchesBuf)], c.batchesBuf) | |||
| c.batches = c.batches[:len(c.batchesBuf)] | |||
There was a problem hiding this comment.
This is a no-op, right? Did it impact performance?
There was a problem hiding this comment.
My guess about this change (but Bryan can confirm or negate), is that we had to do this change because c.batches may need to grow after the change in newMergeIterator(). @bboreham is my understanding correct?
There was a problem hiding this comment.
Yes the append will grow the slice if required, whereas copy will panic. TestMergeIter/DoubleDelta fails if you don't make this change.
| batches: make(batchStream, 0, len(its)*2*promchunk.BatchSize), | ||
| batchesBuf: make(batchStream, len(its)*2*promchunk.BatchSize), | ||
| batches: make(batchStream, 0, len(its)), | ||
| batchesBuf: make(batchStream, len(its)), |
There was a problem hiding this comment.
I don't recall exactly why the pre-allocation was so big - wondering if you know why?
I can't think of a reason why this would affect correctness either, and the perf results speak for themselves...
There was a problem hiding this comment.
From the correctness perspective, this change should be fine. batchesBuf looks to be written only by mergeStreams() which extends the slice if required.
| @@ -112,8 +112,7 @@ func (c *mergeIterator) buildNextBatch(size int) bool { | |||
| for len(c.h) > 0 && (len(c.batches) == 0 || c.nextBatchEndTime() >= c.h[0].AtTime()) { | |||
| c.nextBatchBuf[0] = c.h[0].Batch() | |||
| c.batchesBuf = mergeStreams(c.batches, c.nextBatchBuf[:], c.batchesBuf, size) | |||
| copy(c.batches[:len(c.batchesBuf)], c.batchesBuf) | |||
| c.batches = c.batches[:len(c.batchesBuf)] | |||
There was a problem hiding this comment.
My guess about this change (but Bryan can confirm or negate), is that we had to do this change because c.batches may need to grow after the change in newMergeIterator(). @bboreham is my understanding correct?
* MergeIterator: allocate less memory at first We were allocating 24x the number of streams of batches, where each batch holds up to 12 samples. By allowing `c.batches` to reallocate when needed, we avoid the need to pre-allocate enough memory for all possible scenarios. * chunk_test: fix innacurate end time on chunks The `through` time is supposed to be the last time in the chunk, and having it one step higher was throwing off other tests and benchmarks. * MergeIterator benchmark: add more realistic sizes At 15-second scrape intervals a chunk covers 30 minutes, so 1,000 chunks is about three weeks, a highly un-representative test. Instant queries, such as those done by the ruler, will only fetch one chunk from each ingester. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
What this PR does:
We were allocating 24x the number of streams of batches, where each batch holds up to 12 samples.
By allowing
c.batchesto reallocate when needed, we avoid the need to pre-allocate enough memory for all possible scenarios.Also fix innacurate end time on chunks test data, which was throwing off the benchmark, and add more realistic test sizes - at 15-second scrape intervals a chunk covers 30 minutes, so 1,000 chunks is about three weeks, a highly un-representative test.
Which issue(s) this PR fixes:
Fixes #1195
Benchmarks
Checklist
CHANGELOG.mdupdated