store-gateway: Use a pooling memory allocator for loading chunks from memcached

[56quarters]

Store-gateways spend a significant portion of time reading data from memcached. Due to the way the memcached client used works, each read requires the client to allocate enough space for the data read. This memory is thread thrown away after the request is finished. This creates a lot of garbage and extra work for something only briefly used.

Modeling an approach based on the work done in #3756, we should attempt to pool memory used for reading from the chunks cache to reduce garbage generated and improve request speed.

From a dev cell, ~1/3 of memory allocated is by the memcached client:

---

Tasks

• Create a design document and get consensus on approach
• Add optional caller supplied allocator for grafana/gomemcache - Allow result memory to be allocated from per-call Allocator gomemcache#8
• Replace memcached client in grafana/dskit with grafana/gomemcache - Use Grafana Memcached client fork instead of upstream dskit#248
• Add optional caller supplied allocator for Cache and memcached in grafana/dskit
• Introduce use of caller supplied allocator in Mimir store-gateway
  • Add ability to switch (or disable) allocator implementation used for caching based on config setting
  • Experiment with different allocators

[56quarters]

Some initial notes:

• It makes sense that the allocator is passed to caching methods as a parameter: the caching layer doesn't know about the lifecycle of the objects it's working with. Adding cleanup methods to the caching layer (ala .Put([]byte)) would be hugely invasive.
• In terms of caching, the chunks memcached instances do far more reads (in terms of bandwidth) than index, results, or metadata caches. Thus we should focus on that first within Mimir.
• This will require changes in the gomemcache client, and the dskit wrappers.
• We should probably remove the pooling in gomemcache added here to avoid confusion. Better to have only a single way of doing something.

[charleskorn]

We should probably remove the pooling in gomemcache grafana/gomemcache#5 to avoid confusion. Better to have only a single way of doing something.

Are you thinking we remove the Pool field from Client and instead pass a Pool (or Allocator or whatever we call it) instance to Get(), GetMulti() etc.? Or something else?

If the former, how many other projects use this client? Do we care about making a breaking change like this?

[pracucci]

We should probably remove the pooling in gomemcache grafana/gomemcache#5 to avoid confusion. Better to have only a single way of doing something.

We should talk to Bryan and other projects using that fork (Mimir currently use an older commit not including it). I think that pooling would be very harder to use for the Mimir case, instead of passing the allocator directly to GetMulti() as a functional parameter.

[56quarters]

We should probably remove the pooling in gomemcache grafana/gomemcache#5 to avoid confusion. Better to have only a single way of doing something.

We should talk to Bryan and other projects using that fork (Mimir currently use an older commit not including it). I think that pooling would be very harder to use for the Mimir case, instead of passing the allocator directly to GetMulti() as a functional parameter.

Yeah, let's definitely talk to the other projects using this. I'm not sure if anyone in Grafana is using the pooling behavior yet so it might be pretty easy to remove without disruption.

My $0.02 is that the lifecycle of the memory being used for cache results is very request-oriented so it makes more sense to have the caller control the lifecycle of the memory instead of needing to return pooled bytes to the memecached client. The client will end up hidden behind many layers of abstraction (dskit wrappers, store-gateway wrappers). Additionally, doing via passing the allocator (or whatever we call it) allows us to do arena-style memory management.

[pracucci]

My $0.02 is that the lifecycle of the memory being used for cache results is very request-oriented so it makes more sense to have the caller control the lifecycle of the memory instead of needing to return pooled bytes to the memecached client.

💯

[bboreham]

This idea was raised in #285.
#2831 is an earlier attempt to use it in store-gateway.

[56quarters]

This is incidentally being done as part of #3968. I will focus on testing memory pooling with the index cache. Completed in #4074 while #3968 is being tuned and worked on.