Receiver: Use intern package when reallocating label strings

[matej-g]

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

Changes

This is an alternative to #5899 (please see that PR for rational for this change) but uses the go4org/intern instead of our homebrewed method. The advantage of this is that the go4org/intern package is capable of dropping references to unused string automatically, so we don't have to worry about cleaning up references manually (by e.g. counting usage, dropping references periodically etc.) or worry about keeping stale references, causing a memory build-up over longer time.

The downside is that the package violates safe practices when it comes to operating with pointers. However, the package takes precautions to prevent problems with the program (for full details see: https://mdlayher.com/blog/unsafe-string-interning-in-go/). Moreover, the package is written and maintained by some of the developers that are close to the upstream Go community, who ensure the quality of the code is up to Go's standards. The package is considered mature and stable as per project's description. Considering all of this, I believe it is safe to use the package in Thanos as well.

If people deem this still too risky we can consider our own safe implementation via #5899.

Verification

We (cc @philipgough )have run this in a synthetic load test with k6 load testing tools on a cluster of 6 receivers with replication factor 3. Overall, we could confirm some gains with this change. We clearly keep fewer objects in heap as expected (average by pod for each respective version, graph on the right). On the actual heap size and working set metrics, we could confirm improvement of at least 1-2 GB average per receiver replica (values are in GB, green is latest main, yellow ismain + PR changes):

On local benchmark levels, both label and handler benchmarks seem to remain on tolerable levels (with the exception of extremely large label number i.e. from 100000). This is expected as the gain should come from keeping fewer objects in memory for long term rather then fewer allocations.

name                                                                           old time/op    new time/op    delta
TransformWithAndWithoutCopyWithSingleLabel/ZLabelsToPromLabels-12                1.50ns ± 2%    1.53ns ± 6%      ~     (p=0.841 n=5+5)
TransformWithAndWithoutCopyWithSingleLabel/ZLabelsToPromLabelsWithRealloc-12     50.4ns ± 3%   133.2ns ± 2%  +164.21%  (p=0.008 n=5+5)
TransformWithAndWithoutCopyWith1000Labels/ZLabelsToPromLabels-12                 1.57ns ± 4%    1.52ns ± 2%      ~     (p=0.056 n=5+5)
TransformWithAndWithoutCopyWith1000Labels/ZLabelsToPromLabelsWithRealloc-12      48.5µs ± 3%   161.1µs ± 1%  +232.22%  (p=0.008 n=5+5)
TransformWithAndWithoutCopyWith100000Labels/ZLabelsToPromLabels-12               1.51ns ± 0%    1.51ns ± 1%      ~     (p=0.286 n=4+5)
TransformWithAndWithoutCopyWith100000Labels/ZLabelsToPromLabelsWithRealloc-12    5.44ms ± 3%   34.98ms ±10%  +542.99%  (p=0.008 n=5+5)

name                                                                           old alloc/op   new alloc/op   delta
TransformWithAndWithoutCopyWithSingleLabel/ZLabelsToPromLabels-12                 0.00B          0.00B           ~     (all equal)
TransformWithAndWithoutCopyWithSingleLabel/ZLabelsToPromLabelsWithRealloc-12      96.0B ± 0%     96.0B ± 0%      ~     (all equal)
TransformWithAndWithoutCopyWith1000Labels/ZLabelsToPromLabels-12                  0.00B          0.00B           ~     (all equal)
TransformWithAndWithoutCopyWith1000Labels/ZLabelsToPromLabelsWithRealloc-12      96.0kB ± 0%    96.0kB ± 0%    +0.00%  (p=0.008 n=5+5)
TransformWithAndWithoutCopyWith100000Labels/ZLabelsToPromLabels-12                0.00B          0.00B           ~     (all equal)
TransformWithAndWithoutCopyWith100000Labels/ZLabelsToPromLabelsWithRealloc-12    9.60MB ± 0%    9.67MB ± 1%    +0.69%  (p=0.008 n=5+5)

name                                                                           old allocs/op  new allocs/op  delta
TransformWithAndWithoutCopyWithSingleLabel/ZLabelsToPromLabels-12                  0.00           0.00           ~     (all equal)
TransformWithAndWithoutCopyWithSingleLabel/ZLabelsToPromLabelsWithRealloc-12       2.00 ± 0%      2.00 ± 0%      ~     (all equal)
TransformWithAndWithoutCopyWith1000Labels/ZLabelsToPromLabels-12                   0.00           0.00           ~     (all equal)
TransformWithAndWithoutCopyWith1000Labels/ZLabelsToPromLabelsWithRealloc-12       2.00k ± 0%     2.00k ± 0%      ~     (all equal)
TransformWithAndWithoutCopyWith100000Labels/ZLabelsToPromLabels-12                 0.00           0.00           ~     (all equal)
TransformWithAndWithoutCopyWith100000Labels/ZLabelsToPromLabelsWithRealloc-12      200k ± 0%      203k ± 2%    +1.65%  (p=0.008 n=5+5)

name                                                                                old time/op    new time/op    delta
HandlerReceiveHTTP/typical_labels_under_1KB,_500_of_them/OK-12                         939µs ± 6%     944µs ± 7%     ~     (p=0.548 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_500_of_them/conflict_errors-12           1.12ms ± 6%    1.12ms ± 4%     ~     (p=1.000 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_5000_of_them/OK-12                       9.43ms ± 5%   10.64ms ± 8%  +12.90%  (p=0.008 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_5000_of_them/conflict_errors-12          11.1ms ± 7%    11.9ms ± 9%     ~     (p=0.056 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_20000_of_them/OK-12                      38.0ms ± 5%    38.9ms ± 2%     ~     (p=0.310 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_20000_of_them/conflict_errors-12         44.5ms ± 2%    45.7ms ± 5%     ~     (p=0.548 n=5+5)
HandlerReceiveHTTP/extremely_large_label_value_10MB,_10_of_them/OK-12                 85.0ms ±16%    86.2ms ± 1%     ~     (p=0.151 n=5+5)
HandlerReceiveHTTP/extremely_large_label_value_10MB,_10_of_them/conflict_errors-12    84.1ms ± 8%    85.6ms ± 1%     ~     (p=0.310 n=5+5)

name                                                                                old alloc/op   new alloc/op   delta
HandlerReceiveHTTP/typical_labels_under_1KB,_500_of_them/OK-12                        1.15MB ± 0%    1.15MB ± 0%     ~     (p=0.222 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_500_of_them/conflict_errors-12           1.41MB ± 0%    1.41MB ± 0%     ~     (p=0.056 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_5000_of_them/OK-12                       13.1MB ± 0%    13.1MB ± 0%     ~     (p=0.310 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_5000_of_them/conflict_errors-12          15.5MB ± 0%    15.5MB ± 0%     ~     (p=0.056 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_20000_of_them/OK-12                      53.0MB ± 0%    53.0MB ± 1%     ~     (p=0.413 n=4+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_20000_of_them/conflict_errors-12         62.0MB ± 0%    62.0MB ± 0%     ~     (p=1.000 n=5+5)
HandlerReceiveHTTP/extremely_large_label_value_10MB,_10_of_them/OK-12                  110MB ± 0%     110MB ± 0%     ~     (p=0.730 n=4+5)
HandlerReceiveHTTP/extremely_large_label_value_10MB,_10_of_them/conflict_errors-12     110MB ± 0%     110MB ± 0%     ~     (p=1.000 n=5+5)

name                                                                                old allocs/op  new allocs/op  delta
HandlerReceiveHTTP/typical_labels_under_1KB,_500_of_them/OK-12                         3.10k ± 0%     3.10k ± 0%     ~     (all equal)
HandlerReceiveHTTP/typical_labels_under_1KB,_500_of_them/conflict_errors-12            6.63k ± 0%     6.63k ± 0%     ~     (p=0.333 n=4+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_5000_of_them/OK-12                        30.3k ± 0%     30.3k ± 0%     ~     (p=0.571 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_5000_of_them/conflict_errors-12           65.1k ± 0%     65.2k ± 0%     ~     (p=0.444 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_20000_of_them/OK-12                        121k ± 0%      121k ± 0%     ~     (p=0.738 n=5+5)
HandlerReceiveHTTP/typical_labels_under_1KB,_20000_of_them/conflict_errors-12           260k ± 0%      260k ± 0%     ~     (p=1.000 n=5+5)
HandlerReceiveHTTP/extremely_large_label_value_10MB,_10_of_them/OK-12                   89.6 ± 4%      88.8 ± 1%     ~     (p=0.667 n=5+5)
HandlerReceiveHTTP/extremely_large_label_value_10MB,_10_of_them/conflict_errors-12       220 ± 1%       220 ± 1%     ~     (p=1.000 n=5+5)

[fpetkovski]

This looks quite cool and also simpler than the first implementation. It's interesting that benchmarks show no real difference :)

[douglascamata]

This is awesome, @matej-g! 😮 🙇 🚀

@fpetkovski the benchmarking code manually calls the GC in between calls to the benchmarked function. I suspect this triggers a cleanup of the interned strings as their only refs go away.

[matej-g]

@fpetkovski @douglascamata Honestly I'm not sure, this is one thing I still did not manage to figure fully out. My assumption is that we're still allocating each label string but eventually we're keeping one copy of the string around for long time and garbage collecting the others.

[douglascamata]

@matej-g your statement seems correct to me. We keep a single copy of the interned strings. But the actual strings aren't kept forever in memory forever and will be garbage collected if they have no more references (see https://github.com/go4org/intern/blob/main/intern.go#L141). So the GC run in between benchmark iterations is likely cleaning things up.

You could confirm it by running the code being benchmarked twice for each benchmark iteration. Comparing to running twice without interning, we should be able to see the memory difference.

[saswatamcode]

This looks really cool! 🌟

For the benchmarks, I don't think we manually call GC in the benchmark, but only manually call it after all the benchmark test cases here. But you'd have auto GC cycles running during benchmark anyway!

You can try running the benchmark with GODEBUG=gctrace=1 and see when the GC is working and try to set GC pacing with GOGC. I think there is some nuance to it being (forced) as well. 🙂

[douglascamata]

@saswatamcode internal code in the benchmark package is calling the GC, not our code: https://github.com/golang/go/blob/master/src/testing/benchmark.go#L187

[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.

[GiedriusS]

👍 I took a deeper look and would like to try getting this in. Perhaps this could be behind a feature flag in Thanos Receive?

[matej-g]

@GiedriusS I'm fine with that, sorry for the delays and leaving this unfinished, if this looks good I can sanitise this PR and un-draft it

[GiedriusS]

Yeah, I think let's put this under a feature flag and merge this so others could try it out as well. Thanks for your work 🙇

[matej-g]

Looks like l forgot to undraft, this should now be ready for final approvals, thanks cc @GiedriusS @douglascamata @saswatamcode

[douglascamata]

LGTM! Thanks a lot, @matej-g 🙇

Can't wait to try this one out.

[philipgough]

lgtm

[GiedriusS]

🎖️