Metrics: Revert added locking and annotations - #13567
Merged
moonchen merged 1 commit intoAug 19, 2026
Merged
Conversation
The locking that the thread safety annotations required on the ts::Metrics::Storage read paths introduces too much of a performance regression. Revert it for now, until a better solution can be found. The thread safety analysis infrastructure and its use in SSLOriginSessionCache are unaffected.
moonchen
added
Regression
Revert
Metrics
Performance
and removed
Backport
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ts::Metrics::Storage implementation to roll back previously-added thread-safety annotations and associated locking on some read paths, aiming to avoid a performance regression in metric reads while keeping the rest of the thread-safety infrastructure (e.g., SSLOriginSessionCache) unchanged.
Changes:
- Replaced
ts::mutex/ts::lock_guardusage inMetrics::Storagewithstd::mutex/std::lock_guard. - Removed several thread-safety annotations (
TS_GUARDED_BY,TS_REQUIRES,TS_NO_THREAD_SAFETY_ANALYSIS) fromMetrics::Storage. - Removed locking in some
Storageread helpers (lookup(id, ...),name(id), andvalid(id)), and adjustedrename()locking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/tsutil/Metrics.cc |
Adjusts locking strategy around Storage operations, including removing locks from some ID-based read paths. |
include/tsutil/Metrics.h |
Removes thread-safety annotations and switches Storage to std::mutex, including changing/relaxing locking in small inline helpers. |
Suppressed comments (2)
src/tsutil/Metrics.cc:145
Storage::name()reads_cur_blob,_cur_off, and_blobs[...]without synchronization, but those are updated under_mutexincreate()/createSpan(). This introduces a data race and can lead to returning the wrong name or dereferencing a partially-published blob under concurrent metric creation.
Metrics::Storage::name(Metrics::IdType id) const
{
auto [blob_ix, offset] = _splitID(id);
Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
src/tsutil/Metrics.cc:200
Storage::rename()acquires_mutexonly after reading_blobs[...],_cur_blob,_cur_off, and taking a reference to the stored name. With concurrentcreate()/createSpan()this is a data race, and even without it the late lock undermines the function’s own correctness guarantees. Take the lock before accessing any shared state and remove the later lock_guard.
std::string &cur = std::get<0>(std::get<0>(*blob)[offset]);
std::lock_guard lock(_mutex);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
95
to
99
| Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics::MetricType *out_type) const | ||
| { | ||
| ts::lock_guard lock(_mutex); | ||
| auto [blob_ix, offset] = _splitID(id); | ||
| Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); | ||
|
|
Comment on lines
345
to
349
| valid(IdType id) const | ||
| { | ||
| auto [blob, entry] = _splitID(id); | ||
|
|
||
| ts::lock_guard lock(_mutex); | ||
| return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off))); |
cmcfarlen
approved these changes
Aug 18, 2026
cmcfarlen
left a comment
cmcfarlen
left a comment
Contributor
There was a problem hiding this comment.
Thanks! I know copilot is not happy about it, but we can address the "data race" with atomics in a followup PR.
Contributor
|
Cherry-picked to 10.2.x |
cmcfarlen
pushed a commit
that referenced
this pull request
Aug 19, 2026
The locking that the thread safety annotations required on the ts::Metrics::Storage read paths introduces too much of a performance regression. Revert it for now, until a better solution can be found. The thread safety analysis infrastructure and its use in SSLOriginSessionCache are unaffected. (cherry picked from commit 0201367)
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
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.
The locking that the thread safety annotations required on the
ts::Metrics::Storageread paths introduces too much of a performance regression. Revert it for now, until a better solution can be found.The thread safety analysis infrastructure and its use in
SSLOriginSessionCacheare unaffected.