Allow result memory to be allocated from per-call Allocator

[56quarters]

Remove the existing Client.Pool for pooling memory used by cache results and replace it with an optional per-call Allocator. The per-call allocator allows callers to determine the correct lifecycle for anything allocated.

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

[56quarters]

Note that the Allocator interface here returns []byte as opposed to *[]byte. We use both in Mimir for our various byte pooling functionality. []byte means copying the slice header (24 bytes) when getting something from the pool but can be used without needing to be dereferenced. Anyone have an opinion on what we should do here?

[charleskorn]

Overall LGTM.

One thing that this might be missing: a test to confirm that the allocator set with WithAllocator() is actually used.

[charleskorn]

Note that the Allocator interface here returns []byte as opposed to *[]byte. We use both in Mimir for our various byte pooling functionality. []byte means copying the slice header (24 bytes) when getting something from the pool but can be used without needing to be dereferenced. Anyone have an opinion on what we should do here?

I don't have a strong opinion... have you benchmarked both options to see if there's a significant difference?

[pracucci]

The design LGTM! I also would love to see a benchmark comparison between []byte and *[]byte. Don't focus too much on Mimir store-gateway using []byte in their pool: we can change it if benefit is significant.

[pracucci]

May you show the benchmark comparison before / after, please?

[56quarters]

May you show the benchmark comparison before / after, please?

Old pooling vs new pooling with []byte:

$ benchstat old.txt new.txt 
name                 old time/op    new time/op    delta
ParseGetResponse-16     251ns ± 0%     276ns ± 0%   ~     (p=1.000 n=1+1)

name                 old alloc/op   new alloc/op   delta
ParseGetResponse-16     88.0B ± 0%    112.0B ± 0%   ~     (p=1.000 n=1+1)

name                 old allocs/op  new allocs/op  delta
ParseGetResponse-16      2.00 ± 0%      3.00 ± 0%   ~     (p=1.000 n=1+1)

Old pooling vs new pooling with *[]byte

$ benchstat old.txt new.txt 
name                 old time/op    new time/op    delta
ParseGetResponse-16     251ns ± 0%     255ns ± 0%   ~     (p=1.000 n=1+1)

name                 old alloc/op   new alloc/op   delta
ParseGetResponse-16     88.0B ± 0%     88.0B ± 0%   ~     (all equal)

name                 old allocs/op  new allocs/op  delta
ParseGetResponse-16      2.00 ± 0%      2.00 ± 0%   ~     (all equal)

[pracucci]

The delta reporting is misleading. There's actually a difference.

[56quarters]

Overall LGTM.

One thing that this might be missing: a test to confirm that the allocator set with WithAllocator() is actually used.

Well, it turns out the current unit tests depend on being able to run a local memcached. If there's no memcached binary available (which there isn't because CI runs in a standard ubuntu image), they're skipped. So none of the tests have actually been running: https://github.com/grafana/gomemcache/blob/master/memcache/memcache_test.go#L39

[56quarters]

I've elected to not force the length of the buffer like the previous code did. This is because the interface for Allocator guarantees that the returned buffer will be correctly sized.

[56quarters]

Looking at the old BytesPool interface, it didn't guarantee length, only capacity. WDYT about taking that approach here? Seems a bit less error prone and moves some of the burden onto this code to assert the correct length vs making Allocator implementations do it.

[56quarters]

I've made this change. This also means that we don't have to do an extra 24b allocation in the test allocator (since we already do one in the client when using the pooled slice).

[56quarters]

The tests for the client need a lot of work so I'm going to merge this as-is and will follow up with a test to ensure the allocator is used and some other cleanup.