Memoize tag-based Scope/Counter/Gauge/Timer lookups - #179
Open
th0114nd wants to merge 2 commits into
Open
Conversation
subScope.NewCounterWithTags (and its Gauge/Timer/MilliTimer/ScopeWithTags siblings), plus their statStore-root equivalents, re-derive the full joinScopes/MergeTags/Serialize (or SerializeTags) path on every single call, even when the result is guaranteed to be the same object gostats already handed back last time - a caller that holds a Scope and re-emits the same name+tags repeatedly pays this cost every time. Add a scopeCache to subScope and statStore that memoizes each of these calls keyed on an order-independent hash of (name, tags) (internal/tags.HashNameTags), confirming the stored name+tags on a hit (internal/tags.TagsEqual) before trusting it, so a hash collision falls through to a recompute instead of returning the wrong object. The underlying counters/gauges/timers sync.Maps already retain one object per unique serialized name+tags forever, so this cache's unbounded growth is no different from what gostats already commits to. PerInstance* methods are left uncached, same as their existing MergeTags counterparts weren't touched here either - only the WithTags variants that were previously rebuilding the full key on every call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior design stored the caller's original name+tags alongside each cached value so a hit could be confirmed before being trusted, guarding against a 64-bit hash collision. Since scopeCache never evicts, that verification copy - a map[string]string, which costs on the order of 200+ bytes even for one or two entries, several times the size of the Counter/Gauge/Timer object it's guarding - is retained forever for every distinct metric a process ever emits. For a long-running service with real tag cardinality, that's a meaningful, avoidable memory overhead. Widen the hash to 128 bits (two independent FNV-1a passes over name+tags, computed together in a single pass over the tags map) and drop the stored copy entirely. At 128 bits the birthday-bound collision probability (n^2/2^129) stays negligible for any cardinality a real service would produce, so a hash match can be trusted directly - internal/tags.TagsEqual and the single-hash HashNameTags are removed as no longer needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
th0114nd
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Profiling of a downstream service showed gostats' own scope/tag/counter key
computation (
joinScopes,TagSet.MergeTags,TagSet.Serialize/SerializeTags) accounting for a large share of allocations - even on callsthat are guaranteed cache hits against gostats' own metric registry. The
underlying
Counter/Gauge/Timerobject is already deduplicated by name+tags,but the lookup key for that dedup is rebuilt from scratch on every single
call: join the scope path, merge tag sets, sort, serialize to a string.
This adds a
scopeCachetosubScopeandstatStorethat memoizesScope/ScopeWithTagsand everyNew*WithTagscall on a 128-bit,order-independent hash of
(name, tags). On a cache hit, the call returnsthe previously-created handle directly - no join, no merge, no sort, no
serialize.
Counter/Gauge/Timer per unique serialized name+tags forever, so this
cache's growth is bounded the same way - it isn't a new source of
unbounded memory growth.
128 bits, the birthday-bound collision probability (
n²/2^129) staysnegligible for any cardinality a real service would produce, so a hash
match is trusted directly. An earlier revision stored the caller's
original tags map alongside each entry to confirm a 64-bit hash hit before
trusting it - that copy cost ~200+ bytes per entry (several times the size
of the object it was guarding) and is no longer needed at 128 bits.
NewPerInstanceCounter/Gauge/Timer/MilliTimerare deliberately leftuncached for now - a candidate follow-up once verified safe.
Benchmark results
Synthetic trace simulating a service that holds a long-lived root
Scopebut re-derives nested scopes and tagged metric handles inline on every call
(see the benchmark code in stacked PR #180). Compared via
benchstat,mainvs. this change, n=6:SyntheticTraceSyntheticTraceParallelSyntheticTracePooled*SyntheticTracePooledParallel*SyntheticTracePerInstance(control, uncached path)* "Pooled" reuses the caller's
map[string]stringtag sets across callsinstead of allocating a fresh literal each time, isolating gostats' own
internal allocations from caller-side map-literal cost - this is where the
change goes all the way to 0 allocations, 0 bytes per call on a repeat
(scope, name, tags)lookup. ThePerInstancecontrol - the one call pathintentionally left uncached - shows no change, confirming the improvement
is attributable to the cached paths and not some unrelated speedup.
All results statistically significant (p=0.002,
benchstat).Test plan
go build ./...,go vet ./...,gofmt -l .cleango test ./... -race -count=1- 133 tests passinginternal/tags/tags_test.go(hash order-independence, differinginputs, empty-key/value handling),
stats_cache_test.go(cache-hitidentity across distinct-but-equal tag maps, cache-miss on different tags,
flush-output correctness unchanged, race-checked concurrent access)