pool string builders used in aggregator to concatenate tags#336
pool string builders used in aggregator to concatenate tags#336jstncbllr wants to merge 3 commits into
Conversation
…ncbllr/master # Conflicts: # statsd/aggregator.go # statsd/aggregator_test.go
|
@review codex |
| var sb strings.Builder | ||
| sb := stringBuilderPool.Get().(*strings.Builder) | ||
| defer func() { | ||
| sb.Reset() |
There was a problem hiding this comment.
I have a concern about correctness of this approach.
AFAIR sb.String() returns a string backed directly by the builder's internal buffer, so it does not copy. The defer here resets and returns the builder to the pool before the caller ever uses the returned strings, meaning the next goroutine to get the builder will overwrite the buffer that the returned string values still point into. This is silent heap corruption under concurrent use and might not be caught by the race detector.
There was a problem hiding this comment.
Thanks for digging into this. You were right to be suspicious, though the failure mode isn't quite there. Builder.String() does alias the internal buffer, but Builder.Reset() sets buf = nil rather than truncating, so the returned strings keep sole ownership of the old buffer and the next pool user allocates a fresh one. No corruption; a hammer test under the race detector agrees.
The real problem was one level up: because Reset() drops the buffer, the pool never reused any memory, so this PR didn't actually reduce allocations. Same allocs/op, slightly slower. A heap profile from one of our production services running this patch confirmed it was still allocating on every call. My original benchmark comparison was misleading since getContextAndTagsOld predated the cardinality changes and the two benchmarks weren't building the same string.
I pushed a rework earlier today that builds the key into a pooled byte buffer and probes the map with m[string(buf)], which avoids the allocation on the lookup path. Then I noticed #389 already landed a more complete version of the same idea on master: stack buffers instead of a pool, hashing while appending for shard selection, and a no-tags fast path. So there's nothing left for this PR to do and I'm closing it. Thanks for the review.
|
Hi @jstncbllr , Thank you for suggesting this improvement! |
Replace the strings.Builder pool with a pooled []byte scratch buffer. Builder.Reset() drops the underlying buffer, so pooling builders never reused any memory and every call still paid the Grow allocation. Map lookups written as m[string(buf)] do not allocate (the compiler elides the conversion for map access), so count/gauge/set and the buffered sample path now build the key in the scratch buffer, probe the map, and only materialize a string when inserting a new context, where the key is retained as the map key anyway. Steady-state samples (context already exists) drop from 1 alloc/op to 0 allocs/op. Replaces the getContextAndTagsOld comparison benchmark with steady-state aggregator benchmarks that guard the new behavior.
3 participants
This reduced allocations significantly for us.