Querier doesn't put back bytes in the reusable chunk pool

[krasi-georgiev]

The chunk dir reader uses a pool but never puts anything back to the pool

prometheus/tsdb/chunks/chunks.go

Line 469 in 5ecef35

return s.pool.Get(chunkenc.Encoding(chk[0]), chk[1:1+chkLen])

I think putting back should happen in the Close method

prometheus/tsdb/chunks/chunks.go

Lines 436 to 438 in 5ecef35

	func (s *Reader) Close() error {
	return closeAll(s.cs)
	}

cc @codesome this might be interesting to you since you did some memory optimizations recently 😺

[csmarchbanks]

Relevant PR I did for this a year ago: prometheus-junkyard/tsdb#427, I ended up concluding that it wasn't really worthwhile.

I ran into some subtle bugs that can happen depending on where you try to put chunks back into the pool. Putting a chunk back during close is only slightly helpful since all the intermediate chunks would not be put back.

[codesome]

From a quick search, I found only these 2 places which calls .Chunk(ref)

prometheus/tsdb/querier.go

Line 801 in 5ecef35

c.Chunk, s.err = s.chunks.Chunk(c.Ref)

prometheus/tsdb/compact.go

Line 928 in 5ecef35

chk.Chunk, err = c.chunks.Chunk(chk.Ref)

And the chunk pool is passed on by the DB to the Block. So maybe we can pass the same pool in those iterators and put back the old chunks after every iteration. Not sure how much we will benefit from this than just letter the gc collect it away.

[codesome]

Stumbled upon this while working on head chunks on disk - this would apply to Head as well once we have that work upstream. I'll be attempting to put back into the pool during iteration.

[brian-brazil]

@codesome Is this still relevant?

[codesome]

Yes. Chunks are only taken out of pool for queries on the blocks. Same is the case with mmapping chunks now. I attempted fixing it but it was very tricky. I haven't attempted further after that.

[codesome]

Still relevant, hence reopening

[krasi-georgiev]

closing for no activity