Mimir query engine: split LimitingPool

[charleskorn]

What this PR does

This PR is a refactoring of LimitingPool, which was introduced in #8270.

LimitingPool previously served two purposes:

• tracking the current estimated memory consumption of a query and enforcing the max memory consumption limit
• pooling slices of objects

However, there's no need to couple these two things together, and in fact, doing so makes future changes (eg. incorporating in-memory chunk bytes into the memory limit) more difficult.

This PR splits LimitingPool into two parts: MemoryConsumptionTracker and a number of global LimitingBucketedPools.

Unfortunately this introduced an import cycle between the types and pooling packages, so I've opted to merge them together into the types package.

Performance is unchanged within the margin of error of our benchmarks.

Which issue(s) this PR fixes or relates to

(none)

Checklist

• Tests updated.
• [n/a] Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• [n/a] about-versioning.md updated with experimental features.

[jhesketh]

Mostly concerned about the use of globals, otherwise lgtm.

We could perhaps keep some kind of LimitingPool that allows us to reach into the MemoryConsumptionTracker for chunks etc in the future.

[jhesketh]

should we reach into the pool and check the capacity of the slices here? Even though we reject the Get due to the limit, we still end up performing a Get and Put against the pool so it's possible the slice has grown.

[charleskorn]

Not sure I'm following sorry - are you suggesting we should check that the slice we took from the pool has been returned before we returned the error?

[jhesketh]

Yes, although I'm not entirely sure there's a trivial way to do that.
Additionally it's interesting to confirm that the capacity grew even though we didn't use it.

[charleskorn]

Yes, although I'm not entirely sure there's a trivial way to do that.

One way to do it would be to introduce an interface for BucketedPool so we can mock it out in the test and confirm that the slice is returned, but that will introduce a performance overhead for the production code which I'd prefer to avoid.

The consequences of not returning the slice to the pool are relatively minor, especially given we don't expect to hit the limit too often.

Additionally it's interesting to confirm that the capacity grew even though we didn't use it.

Which capacity are you referring to here?

[jhesketh]

Which capacity are you referring to here?

Unless I'm misunderstanding, the capacity of the underlying slice will be increased by the Get, but then not used because it is Put back. (I'm not sure there is much value checking here tbh)

[charleskorn]

Get creates a new slice (or takes it from the pool) with a particular capacity, and Put puts that slice back in the pool. If something else later needs a slice of the same capacity of the discarded slice, it'll be given that slice, unless GC collects it first.