Reduce lock contention in SyncMetricStorage for concurrent metric recording

[ThomsonTan]

SyncMetricStorage guards every RecordLong/RecordDouble call with a single SpinLockMutex over the entire AttributesHashMap. When multiple threads share one instrument, all measurements serialize on this lock — including the heavy MetricAttributes construction from KeyValueIterable.

The BM_MeasurementsThreadsShareCounterTest benchmark confirms this: shared-counter throughput is significantly worse than per-thread counters.

The lock granularity could be improved.

References

• Lock site: sync_metric_storage.h
• AttributesHashMap: attributes_hashmap.h
• SpinLockMutex: spin_lock_mutex.h

[github-actions]

This issue is available for anyone to work on. Make sure to reference this issue in your pull request.
✨ Thank you for your contribution! ✨

[perhapsmaple]

I'd like to work on this issue if it's available

[lalitb]

The bottleneck here is that a single lock covers both MetricAttributes construction and the hashmap lookup/update path, so even threads recording to completely unrelated attribute sets serialize on the same lock.

In particular, SyncMetricStorage::RecordLong/RecordDouble takes the storage lock before calling AttributesHashMap::GetOrSetDefault(const KeyValueIterable&, ...), and that overload builds MetricAttributes{attributes, attributes_processor} internally. So the relatively expensive attribute materialization work currently happens while the storage lock is held. In steady state, the common case should be updating an existing aggregation rather than inserting a new key, so shortening the critical section on that path is probably the highest-impact change. The benchmark mentioned in the issue (BM_MeasurementsThreadsShareCounterTest) is the right place to validate that.

A C++14-friendly path forward:

1. Move MetricAttributes construction outside the lock.
   The existing GetOrSetDefault(MetricAttributes&&, ...) overload already supports passing a pre-built key, so this looks like a small and safe first step. It also directly addresses the existing TODO in attributes_hashmap.h.

2. If more reduction is needed, shard/stripe the map.
   Build the attributes first, pick a shard from the hash, and lock only that shard for lookup/insert/update. Collect() can then rotate all shards in a coordinated way so the collection boundary stays well-defined. That should reduce contention for threads hitting different attribute sets without changing the overall storage model. (A reader-writer lock is also worth considering for the lookup path, e.g. std::shared_timed_mutex or a custom C++14 implementation, though after step 1 the critical section may be short enough that a sharded spinlock wins on coordination overhead.)

One thing worth being careful about: moving Aggregate() fully outside map synchronization is not just a pointer-lifetime question. Since SyncMetricStorage::Collect() rotates the active map, we also need to define what happens if a measurement lands on the just-rotated-out map. That looks like a collection-window semantics question as much as a threading one, so I would treat that as a separate step rather than bundling it into the initial contention reduction.

[lalitb]

@perhapsmaple - thanks , assigned. I have added few suggestions for this in earlier comment.

One thing to be careful about: reducing lock hold time is the goal here, but please preserve the current Collect() / active-map rotation semantics. In particular, moving MetricAttributes construction out of the lock looks like a safe first step, but moving Aggregate() fully outside map synchronization needs extra care because Collect() swaps the active map.

A good incremental approach would be:

1. first move attribute materialization out of the locked section
2. validate against the existing benchmark (BM_MeasurementsThreadsShareCounterTest)
3. if more reduction is needed, consider sharding/striping the map as a follow-up

That keeps the change easier to reason about and review.