store: reduce memory footprint for chunks queries #3937
Conversation
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
krya-kryak
force-pushed
the
experiment-reduce-store-mem
branch
from
4eaa961
to
8c0d7c6
Compare
Linter failed with: `saviour` is a misspelling of `savior` (misspell) Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
krya-kryak
force-pushed
the
experiment-reduce-store-mem
branch
from
d59543d
to
6818d9a
Compare
pkg/store/bucket.go
Outdated
| chunks: map[uint64]chunkenc.Chunk{}, | ||
| } | ||
| } | ||
|
|
||
| // addPreload adds the chunk with id to the data set that will be fetched on calling preload. | ||
| func (r *bucketChunkReader) addPreload(id uint64) error { | ||
| func (r *bucketChunkReader) Chunk(id uint64) (chunkenc.Chunk, error) { |
There was a problem hiding this comment.
Chunk and Close methods are unchanged; just moved exported functions closer to receiver definition.
There was a problem hiding this comment.
Actually, scratch that. First of all, I've broken Chunk() while changing preload flow. Secondly, Chunk() is no longer used, so I've deleted it altogether.
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
| @@ -1318,8 +1319,6 @@ func benchBucketSeries(t testutil.TB, skipChunk bool, samplesPerSeries, totalSer | |||
|
|
|||
| if !t.IsBenchmark() { | |||
| if !skipChunk { | |||
| // Make sure the pool is correctly used. This is expected for 200k numbers. | |||
| testutil.Equals(t, numOfBlocks, int(st.chunkPool.(*mockedPool).gets.Load())) | |||
There was a problem hiding this comment.
Checking the number of slabs allocated by chunk pool is now untrivial as it depends on the time series.
Failing e2e test |
There was a problem hiding this comment.
Thanks for your work! It seems like you have mixed up the order of two benchmarks in your PR's description. The times have decreased, not increased, right? Also, I was able to reproduce the CI failure locally :( Will attach more info once I am able to find something
CHANGELOG.md
Outdated
| @@ -19,8 +19,11 @@ We use _breaking :warning:_ to mark changes that are not backward compatible (re | |||
| ### Fixed | |||
| - [#3204](https://github.com/thanos-io/thanos/pull/3204) Mixin: Use sidecar's metric timestamp for healthcheck. | |||
| - [#3922](https://github.com/thanos-io/thanos/pull/3922) Fix panic in http logging middleware. | |||
| - [#3937](https://github.com/thanos-io/thanos/pull/3937) Store: Fix race condition in chunk pool. | |||
| - [#3937](https://github.com/thanos-io/thanos/pull/3937) Testutil: Fix race condition encountered during benchmarking. | |||
There was a problem hiding this comment.
We typically don't add non user-facing changes here :P
There was a problem hiding this comment.
It seems like you have mixed up the order of two benchmarks in your PR's description. The times have decreased, not increased, right? Also, I was able to reproduce the CI failure locally :( Will attach more info once I am able to find something
time/op did in fact increase in some benchmarks (and went down a bit in the others), it's mostly allocated bytes / op I was aiming here. I'll look into possibility of shaving some cpu time (larger slabs, probaly).
pkg/store/bucket.go
Outdated
| c, ok := r.chunks[id] | ||
| if !ok { | ||
| return nil, errors.Errorf("chunk with ID %d not found", id) | ||
| func (r *bucketChunkReader) savior(b []byte) ([]byte, error) { |
There was a problem hiding this comment.
I find the terminology here interesting. Maybe a simple name of allocate() would suit it more? I'm not sure who/what is being saved from what 😄
Also, we could potentially probably save even allocations more because this only checks the last r.chunkBytes. So, it should perform worse with allocations of size:
42 12345 1 2 3 4 5
i.e. where their sizes are decreasing because we will always allocate more whereas we could potentially store them at the beginning unless I am mistaken. Maybe it would be worth making this change & putting together a small benchmark to see if it optimizes things even more?
There was a problem hiding this comment.
yes, there is a way to optimize this further - as currently slab "tails" are wasted. This would potentially save up to max(raw values chunk / subchunk size) - 1 per slab (set to 64k in this PR). What is that, 3200 bytes? If so, maximum gain is 4.9% per slab.
However, for the benchmarks we have (filled with random values), average size tends to be less than half of that, so that would cut our estimated maximum profit to, say, 1500 bytes per slab (2.3%). Given that not every case would be that bad, I would estimate actual memory wastes to be around 1.5%.
Moreover, we would need to be lucky and only write chunks that are small enough to be written to the wasted space - so we would not be able to reclaim all of it. All in all, I would consider it a win if we reduce that waste from 1.5% to 1%. So, 5MB save on a 1GB allocation. Do we have bigger fish to fry?
There was a problem hiding this comment.
Makes sense, also - one thing at the time (:
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
This reverts commit 194d234. Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
pkg/store/bucket.go
Outdated
| chunks: map[uint64]chunkenc.Chunk{}, | ||
| } | ||
| } | ||
|
|
||
| // addPreload adds the chunk with id to the data set that will be fetched on calling preload. | ||
| func (r *bucketChunkReader) addPreload(id uint64) error { | ||
| func (r *bucketChunkReader) Chunk(id uint64) (chunkenc.Chunk, error) { |
pkg/store/bucket.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // appPreload adds the chunk with id to the data set that will be fetched on calling preload. |
There was a problem hiding this comment.
| // appPreload adds the chunk with id to the data set that will be fetched on calling preload. | |
| // addPreload adds the chunk with id to the data set that will be fetched on calling preload. |
There was a problem hiding this comment.
Can you update this with i,j meanings too?
There was a problem hiding this comment.
Done. Also, renamed addPreload() / preload(), as nothing is preloaded anymore.
pkg/store/bucket.go
Outdated
| c, ok := r.chunks[id] | ||
| if !ok { | ||
| return nil, errors.Errorf("chunk with ID %d not found", id) | ||
| func (r *bucketChunkReader) savior(b []byte) ([]byte, error) { |
There was a problem hiding this comment.
Makes sense, also - one thing at the time (:
krya-kryak
force-pushed
the
experiment-reduce-store-mem
branch
from
9cddbfd
to
18d9fed
Compare
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
krya-kryak
force-pushed
the
experiment-reduce-store-mem
branch
from
18d9fed
to
b6379f7
Compare
krya-kryak
changed the title
krya-kryak
force-pushed
the
experiment-reduce-store-mem
branch
from
b6379f7
to
91a52a4
Compare
There was a problem hiding this comment.
LGTM, all comments have been addressed, is this still a WIP @krya-kryak? I would like to merge this nice optimization. Plus, I couldn't reproduce e2e test failures anymore so they are either flaky or GH Actions runners had some issues
krya-kryak
changed the title
@GiedriusS, thank you. I've set WIP with thoughts of optimizing CPU usage ( #3937 (comment) ). Unfortunately, I was not able to dedicate some time to it this week :( |
Given that most, if not all, CPU usage happens at the side of Thanos Query, and the increase here is so minuscule in comparison with what happens on the Thanos Query side, I think that merging this as-is should be OK. But, let's wait for the opinion of others. |
Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>
krya-kryak
force-pushed
the
experiment-reduce-store-mem
branch
from
cc48150
to
4b42644
Compare
Changes
two data racesone data race found during work on the main part.Verification
A
DownsampledBlockSeriesbenchmark was added. Note the reduction in allocated memory amount.