Receive: Reuse strings in writer, scoped to a request

[matej-g]

Signed-off-by: Matej Gera matejgera@gmail.com

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

This is a quick idea inspired by some recent discussions on label optimizations and string interning.

It comes from assumption that even in a single write request, on average, there ought to be at least some label names and / or values that will be repeated. I good example is the label name __name__ which every series is guaranteed to have. Other good candidates can be label names job, instance etc. which will repeat in multiple series with high probability.

The idea is to intern these strings, but only on the level of a single write request. We already do reallocation of these strings to detach them from the larger gRPC request, so we might reuse strings that repeat themselves in either label name or value. Doing this on a per request level also means we do not need to track string usage globally, which although would be even more efficient, is more tricky to implement (see the Prometheus issue ref below).

While this does not give us 100% string interning when it comes to TSDB as a whole, it could already save us some long-term string allocations which are held in TSDB's memory. Summing up all smaller allocation savings on each request could still translate into overall lower memory usage on receiver, at least until Prometheus has figured string interning on their side (reference: prometheus/prometheus#11133) when such mechanism would be part of the TSDB itself. Although this introduces some overhead at the time of string reallocation, this trade off might be acceptable from the point of long-term memory usage of receiver.

Verification

Test are still passing. I also have tried running this locally on a kind cluster and all seem working fine. If this idea is deemed valid, I'll try few more longer term test runs / benchmarks, to see how it fairs in comparison to latest Thanos release.

On benchmarks, with an extreme test case (100 series, each with a single label, where label name == label value - meaning we'll always intern this to single string), the allocations have decreased as expected, but the overhead of interning is more pronounced:

name                                                           old time/op    new time/op    delta
TransformWithAndWithoutCopy/ZLabelsToPromLabels-12               12.8ns ± 0%    10.1ns ± 0%   ~     (p=1.000 n=1+1)
TransformWithAndWithoutCopy/ZLabelsToPromLabelsWithRealloc-12    6.58µs ± 0%   19.84µs ± 0%   ~     (p=1.000 n=1+1)

name                                                           old alloc/op   new alloc/op   delta
TransformWithAndWithoutCopy/ZLabelsToPromLabels-12                0.00B          0.00B        ~     (all equal)
TransformWithAndWithoutCopy/ZLabelsToPromLabelsWithRealloc-12    9.60kB ± 0%   14.76kB ± 0%   ~     (p=1.000 n=1+1)

name                                                           old allocs/op  new allocs/op  delta
TransformWithAndWithoutCopy/ZLabelsToPromLabels-12                 0.00           0.00        ~     (all equal)
TransformWithAndWithoutCopy/ZLabelsToPromLabelsWithRealloc-12       200 ± 0%       108 ± 0%   ~     (p=1.000 n=1+1)

[fpetkovski]

Do we require a lock here, or is the cache scoped to a single request?

[matej-g]

For the reasons stated in the PR description, this is only scoped to single request. Probably a good idea to document this in the method / type description as well.

[fpetkovski]

Would you mind pointing out what is the exact reason again? I re-read the description but I could not understand what's the problem with a cross-request cache. I also looked at the linked Prometheus issue, but that just says that the way we store labels will change. Is that the reason why we don't want to go beyond a single request?

[matej-g]

Sorry, looks like I mistakenly referenced something that's not really in that issue. I'll update the description.

What I'm unsure about is how we'd do this with one global cache, as we would not know when we can drop the reference from the map, which could potentially lead to continuous growth of map (in environments with greater series churn) and it would retain 'stale' strings.

@philipgough suggested we could have some sort of of TTL and periodically drop references from map, which sounds like one potential solution (worst case scenario we recreate any strings that were previously removed form the cache). He also pointed me to https://github.com/go4org/intern which I'm also trying out, as it is capable of 'automagically' dropping entries that are no longer referenced, which would also sidestep the issue.

[fpetkovski]

Got it. Yeah a TTL does make sense. Maybe we can start with just resetting the map and then we can add some smart tracking? Doing a reset every 1h should be good enough I think, but we can experiment with it. Also that library looks neat, let's 🤞 it works as advertised.

[matej-g]

I created an alternative PR with usage of the intern package here #5926

[stale]

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

[matej-g]

This is superseded by #5926, closing.