Set minimum buffer size of BytesPool to maxChunk size

[jabbrwcky]

This PR fixes #1580

It fixes the immediate issue, but it could be worthy to discuss if pool.BytesPool is necessary at all or could be replaced by a simpler direct usage of sync.Pool

• Match store bytes pool size to size of chunks used by chunkByteReader.

Changes

• extract constant nested in method to package level
• change pool.BytesPool construction to use chunk size as smallest bucket size

Verification

The store does no longer exhaust its byte pool size on larger buckets

[GiedriusS]

I think this makes sense, thanks!

[bwplotka]

This is what we end up with:

Had: Sizes: [200000 400000 800000 1600000 3200000 6400000 12800000 25600000] overall: 8
Now: Sizes: [16000 32000 64000 128000 256000 512000 1024000 2048000 4096000 8192000 16384000 32768000] overall: 12

More sizes we have it defeats the purpose of sync pool indeed. But for queries that uses lot's of single chunks (e.g especially for downsampled data) having smaller pools might make sense. Any chance for some benchmarks before & after? (: In terms of memory usage for the same requests?

Otherwise LGTM 👍

[jabbrwcky]

More sizes we have it defeats the purpose of sync pool indeed. But for queries that uses lot's of single chunks (e.g especially for downsampled data) having smaller pools might make sense. Any chance for some benchmarks before & after? (: In terms of memory usage for the same requests?

The memory usage is only positively affected as described in the linked issue.

The main issue is that the individual chunk files are fetched in parallel in 16k byte parts when the chunk is read.

So this one:

would be fetched in ~ 1,244 concurrent requests - each downloader requests 16k from the byte pool which caused in the previous version to grab 248.75 MB from the byte pool (because the 16k request was allocated the smallest buffer size: 200k), with the changed version it is only 19,904 MB (smallest buffer: 16k)

The issue appears on primarily when quering as far as I can tell, because the byte chunks are allocated in

thanos/pkg/store/bucket.go

Line 1272 in 4c7ecd3

c, err := b.chunkPool.Get(int(length))

and only returned when the reader is closed in

thanos/pkg/store/bucket.go

Line 1887 in 4c7ecd3

r.block.chunkPool.Put(b)

For some of our index chunk files with 500+MB this amounted to a required pool size of 6.25GB to successfully read that chunk file, which blew our Max BytePool size of 4GB, resulting in the byte pool exhausted error when querying.

With the change it 'only' allocates the chunk size (plus a few bytes if the chunk is not evenly divisible by 16k) for reading the 500MB chunk, which is a lot better and fixes the issue for us for now

[bwplotka]

OK I think there are few misunderstandings but overall this fix makes sense (:

• 000001 chunk file stores multiple chunks (!). At least one for each series.
• Partitioning tries to compose request against bucket (and that's the byte slice we take) into bigger one:

  thanos/pkg/store/bucket.go

  Line 1789 in 1389096

  parts := r.block.partitioner.Partition(len(offsets), func(i int) (start, end uint64) {

  This means that we will request a minimum 16k ONLY if for some reason our query touches only single chunk (it can happen though for short time range queries and when we touch downsampled data).

would be fetched in ~ 1,244 concurrent requests - each downloader requests 16k from the byte pool which caused in the previous version to grab 248.75 MB from the byte pool (because the 16k request was allocated the smallest buffer size: 200k), with the changed version it is only 19,904 MB (smallest buffer: 16k)

Partitioning makes this really unlikely - however still you can exhaust pool with multiple specific queries at the same time.

and only returned when the reader is closed in

Yes - which is done once the requests finish so it should be fine (?)

For some of our index chunk files with 500+MB this amounted to a required pool size of 6.25GB to successfully read that chunk file, which blew our Max BytePool size of 4GB, resulting in the byte pool exhausted error when querying.

That makes sense, but I guess increasing the limit (and memory) will work.

With the change it 'only' allocates the chunk size (plus a few bytes if the chunk is not evenly divisible by 16k) for reading the 500MB chunk, which is a lot better and fixes the issue for us for now

This means to me that your queries are hitting small chunks a lot as I mentioned before.