Add optional buffer pool for item values

[LeviHarrison]

This PR adds support for an optional Bytes pool to recycle the buffers used to hold cache item values. A "release" or "return" method is not provided as it is expected that the caller will have access to the pool itself.

name                 old time/op    new time/op    delta
ParseGetResponse-40    2.51µs ± 0%    2.59µs ± 0%   +3.03%  (p=1.000 n=1+1)

name                 old alloc/op   new alloc/op   delta
ParseGetResponse-40    4.32kB ± 0%    4.32kB ± 0%   -0.05%  (p=1.000 n=1+1)

name                 old allocs/op  new allocs/op  delta
ParseGetResponse-40      6.00 ± 0%      5.00 ± 0%  -16.67%  (p=1.000 n=1+1)

Ref: grafana/mimir#285

[bboreham]

Thanks for the PR.

If we're going to make this change, it should be to reduce garbage-collection.
Your benchmark stats show zero change in bytes allocated, which is the main driver of GC overhead. Also a 3% slowdown. Suggests something is wrong, either with the code or the benchmark.

[LeviHarrison]

Suggests something is wrong, either with the code or the benchmark.

Indeed there was, a new reader was being allocated for every iteration. Fixed that, and here are the new numbers:

name                 old time/op    new time/op    delta
ParseGetResponse-40     481ns ± 0%     471ns ± 0%   -2.10%  (p=1.000 n=1+1)

name                 old alloc/op   new alloc/op   delta
ParseGetResponse-40     96.0B ± 0%     88.0B ± 0%   -8.33%  (p=1.000 n=1+1)

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

[bboreham]

why +2 ?

[LeviHarrison]

Because the buffers are allocated with a capacity of 2 more than the data size to accommodate for the terminating \r\n (not included in the size metadata in the response). So for the case of our test pool, it will need to provide buffers than are 2 bigger than the data size.

gomemcache/memcache/memcache.go

Line 510 in 3a2f55c

it.Value = make([]byte, size+2)

[bboreham]

I see you moved this up to the call, which seems more sensible to me.
Sorry, I misunderstood.
It doesn't matter hugely, since this is a test, but either passing in +2 from somewhere that knows the right size, or changing Get(sz) to call make when the pool is empty, would be better than hard-coding a +2 in the middle of the pool.

[LeviHarrison]

Ok, I moved it up.

[LeviHarrison]

Now:

name                 old time/op    new time/op    delta
ParseGetResponse-40     780ns ± 0%     510ns ± 0%  -34.63%  (p=1.000 n=1+1)

name                 old alloc/op   new alloc/op   delta
ParseGetResponse-40      600B ± 0%       88B ± 0%  -85.33%  (p=1.000 n=1+1)

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

[bboreham]

Thanks, looks good to go now.