perf(ddtrace/tracer): snapshot config in StartSpan to avoid RWMutex contention#4713
Conversation
…ontention StartSpan was acquiring internal/config's RWMutex 13 times per call (once per field getter). Under concurrent load, the atomic ops on the mutex's reader counter cause cache-line bouncing across cores, making the read path scale negatively with concurrency. Add Config.SpanStartSnapshot() which returns the StartSpan-relevant fields under a single RLock, and use it from tracer.StartSpan and applyPPROFLabels. Lock acquisitions per StartSpan drop from 13 to 2 (snapshot + the parameterized ServiceMapping lookup). BenchmarkStartSpanConcurrent-8: 938 ns/op -> 367 ns/op (-60.9%) Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: c25d80c | Docs | Datadog PR Page | Give us feedback! |
Separate file for the snapshot pattern. config.go is dominated by per-field getter/setter pairs; snapshots are conceptually distinct and likely to grow as more hot paths adopt the pattern. Top-of-file comment establishes the per-caller convention so future snapshots have an obvious home. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ry surfaces Add a one-liner to the Config struct doc comment and a subsection in the package README so an agent or developer exploring this package finds the snapshot pattern from the entry points they're likely to hit, rather than needing to open snapshots.go from the top. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
BenchmarksBenchmark execution time: 2026-05-01 21:35:31 Comparing candidate commit c25d80c in PR branch Found 7 performance improvements and 0 performance regressions! Performance is the same for 194 metrics, 0 unstable metrics.
|
genesor
left a comment
genesor
left a comment
There was a problem hiding this comment.
Great work! I like the approach a lot
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
c28e04a
into
main
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
3 participants
What does this PR do?
Adds
Config.SpanStartSnapshot()tointernal/config, which returns all the config fields read bytracer.StartSpanunder a singleRLock.StartSpanandapplyPPROFLabelsare updated to read from the snapshot instead of calling individual getters. Lock acquisitions perStartSpandrop from 13 → 2 (snapshot +ServiceMapping(span.service), which can't be folded in because the lookup key isn't known until after the snapshot is read).Motivation
Each per-field getter on
internal/config.Configacquires the samesync.RWMutexfor read. These don't block each other, but everyRLock/RUnlockdoes an atomic op on the mutex's internal reader counter — when StartSpan is invoked at high concurrency -- which it very well may be in many environments -- the counter cache line bounces across cores, so the read path scales negatively with parallelism.Benchmarks (Apple M3 Max, 5 runs,
-benchtime=3s, viabenchstat):Allocation count and bytes/op are unchanged (15 allocs/op, ~1.5 KiB/op) across all four benchmarks. The single-goroutine cases barely move because uncontended atomics are cheap; the win is concentrated in the multi-core concurrent case (~2.5× speedup), which is exactly what we were targeting.
Reviewer's Checklist
./internal/config/...and./ddtrace/tracer/...pass under-race).BenchmarkStartSpan/BenchmarkStartSpanConcurrent).