Improve client performance#389
Conversation
791ee40 to
1771d5c
Compare
| var contextBuffer [largeContextBufferSize]byte | ||
| return a.gaugeWithContextBuffer(contextBuffer[:0], name, value, tags, cardinality, cardString) | ||
| } | ||
| return a.gaugeWithContextBuffer(make([]byte, 0, contextLen), name, value, tags, cardinality, cardString) |
There was a problem hiding this comment.
Would it make sense to add some tests to make sure we correctly aggregating metrics with different context sizes?
There was a problem hiding this comment.
I don't think so. In the very end we append to a slice; once we exceed the capacity, Go runtime allocates a new backing array on the heap.
| } else { | ||
| contextBuffer, _ = appendContext(contextBuffer, name, tags, cardString) | ||
| } | ||
| shard := &a.countShards[getShardIndexFromHash(a.shardsCount, contextHash)] |
There was a problem hiding this comment.
Code above this line is the same for all flavors. Would we loose on performance if we extracted this repeated bit into a method?
Same question for per-shard code below, which is repeated in countWithStringContext.
There was a problem hiding this comment.
Unfortunately yes, I tried to compact those common pieces into shared functions and got hit by a performance penalty. If I understand the problem correctly, Go compiler is limited in optimizing deep nested function calls.
1771d5c to
0876607
Compare
vickenty
left a comment
There was a problem hiding this comment.
Are getContext and getContextAndTags dead code now?
1789f89 to
97a3531
Compare
| if len(tags) == 0 { | ||
| if cardString == "" { | ||
| return name, "" | ||
| return len(name) | ||
| } | ||
| return name + nameSeparatorSymbol + cardString, "" | ||
| return len(name) + len(nameSeparatorSymbol) + len(cardString) |
There was a problem hiding this comment.
These branches (and matching ones in appendContext and appendContextAndHash) seem to be lacking test coverage. Would it be possible to add it?
Add benchmark coverage for aggregator construction, sharding, steady-state sampling of existing contexts, and sparse/empty flushes.
Store count, gauge, and set shards inline in the aggregator instead of allocating each shard separately. This removes one allocation per shard while keeping shard access by pointer at the call sites that need to lock or mutate the shard. Also leave shard maps nil until the first sample is inserted, and reset flushed maps back to nil. Empty flush intervals no longer allocate replacement maps, while the write path still initializes the map under the shard lock before storing a new metric context.
Build metric contexts directly into byte buffers on the hot aggregation paths and compute the FNV-1a hash while appending those bytes to avoid unneeded materialization of a context string just to hash it for shard selection. The no-tags/no-cardinality path keeps using the metric name directly, avoiding a duplicate context string for the common case. Tagged contexts use bounded stack buffers before falling back to heap allocation. Buffer lenghts were chosen basing on the distribution of typical payloads. *IMPORTANT:* buffered-context code is intentionally duplicated for performance reasons. Signed-off-by: Mark Kirichenko <mark.kirichenko@datadoghq.com>
Pad each count, gauge, and set shard to 128 bytes so adjacent shard mutexes do not share a cache line. RLock updates the RWMutex reader count, so densely packed shards can still bounce cache lines between cores even when goroutines are sampling different shards. We use 128 because currently it is either 64 for x86_64 and most arm64, or 128 for Apple Silicon or some server ARM chips. Signed-off-by: Mark Kirichenko <mark.kirichenko@datadoghq.com>
97a3531 to
7c5de31
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: 6270577 | Docs | Datadog PR Page | Give us feedback! |
ef9982a to
1916f22
Compare
Remove dead code and its tests, and replace it with tests covering new code paths. Signed-off-by: Mark Kirichenko <mark.kirichenko@datadoghq.com>
1916f22 to
5dbbf28
Compare
Exclude the test runs which fail on Macos due to a known linker bug: https://tip.golang.org/doc/go1.24#linker Signed-off-by: Mark Kirichenko <mark.kirichenko@datadoghq.com>
e6dcb7a to
6270577
Compare
This PR introduces a series of changes aimed to improve performance of the Go client.
Overview
More detailed overview
Change 0 - New benchmark
Because we need more numbers to judge the changes.
Change 1 - New shard allocation and placement logic
Improve aggregator's construction and flush. Previously each shard allocated its own map on construction and its own slice on every flush cycle. Both are now amortised/eliminated. Steady-state sampling is unchanged.
Construction — allocs/op (deterministic)
Construction — B/op and sec/op
Flush — allocs/op (deterministic)
Flush — sec/op and B/op
Geomean (sec/op): −5.2% · Geomean (allocs/op): −18.6%
Change 2 - Optimize metric contexts builds
Improves the steady-state sampling hot path a.k.a. the code executed every time a metric is recorded against an existing context. Previously this allocated on every call for most tag counts; after this commit it is zero-allocation for Tags ≤ ~60.
AggregatorSampleExisting — allocs/op (deterministic)
All shard counts, Tags_5 through Tags_60: 3 allocs → 0
Tags_100: unchanged at 3 allocs (large tag sets still escape to heap)
AggregatorSampleExistingByMetricType — allocs/op
Count / Gauge / Set: 1 alloc → 0
BufferedMetricContextsSampleExisting — allocs/op
All shapes below Tags_100: 1 alloc → 0 (Tags_100: 1 → 0 as well)
AggregatorSampleExisting — sec/op (representative: Shards_8)
AggregatorSampleExisting — sec/op (Shards_16, showing tag-count scaling)
AggregatorSampleExistingByMetricType — sec/op
BufferedMetricContextsSampleExistingByShape — sec/op
Geomean (sec/op): −20.2%
Change 3 - Pad aggregator shards to avoid false sharing
Use cache-line padding on shard structs to eliminate false sharing between goroutines on different CPUs accessing different shards.
AggregatorSharding — sec/op
AggregatorSampleExisting at low shard counts — sec/op
False-sharing removal shows through in sampling too, especially at Shards_1–4:
AggregatorSampleExistingByMetricType — sec/op
Geomean (sec/op): −10.2%
Trade-offs / regressions
Downside: Padding inflates the in-memory size of each shard struct, so construction memory grows proportionally to shard count.
This memory is allocated once at client construction and held for the lifetime of the
client, so the impact is negligible for long-running processes. At 256 shards the
footprint grows by ~68 KB.
Benchmark environment: