Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metric: remove mutex around windowed histogram update #140409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhartunian
Copy link
Collaborator

@dhartunian dhartunian commented Feb 3, 2025

Previously, we used an RWMutex to serialize access to the windowed histogram. This was done because there's a rotation that needs to happen that moves the currently active histogram into the prev variable and creates a fresh one to update in cur. This is done to maintain the window.

This change limits the mutex usage to rotation and window snapshotting, removing the need to take a read lock during updates since we can atomically grab the cur histogram.

There's a bit of nuance to manage around the fact that prev can be nil on the first iteration, since cur is the first histogram. After a single rotation, prev will not be nil again.

The prometheus histogram itself (what's stored in the atomic.Value) is thread-safe.

old:  https://github.com/dhartunian/cockroach/commit/507c68f8abb0c9a3f527c66cb42129c2cb096e7d Merge https://github.com/cockroachdb/cockroach/pull/140092
new:  https://github.com/dhartunian/cockroach/commit/ff9dd529682dbc6d485687e5451149c28bd82fd7 metric: remove mutex around windowed histogram upd
args: benchdiff "./pkg/util/metric" "-b" "-r" "BenchmarkHistogramRecordValue" "-c" "10" "--preview"

name                                     old time/op    new time/op    delta
HistogramRecordValue/insert_zero-10        30.7ns ± 8%    24.3ns ± 2%  -20.94%  (p=0.000 n=9+8)
HistogramRecordValue/insert_integers-10    37.4ns ± 6%    33.8ns ± 1%   -9.83%  (p=0.000 n=8+8)
HistogramRecordValue/random_integers-10    44.9ns ± 9%    41.7ns ± 1%   -7.25%  (p=0.000 n=8+8)

name                                     old alloc/op   new alloc/op   delta
HistogramRecordValue/insert_integers-10     0.00B          0.00B          ~     (all equal)
HistogramRecordValue/insert_zero-10         0.00B          0.00B          ~     (all equal)
HistogramRecordValue/random_integers-10     0.00B          0.00B          ~     (all equal)

name                                     old allocs/op  new allocs/op  delta
HistogramRecordValue/insert_integers-10      0.00           0.00          ~     (all equal)
HistogramRecordValue/insert_zero-10          0.00           0.00          ~     (all equal)
HistogramRecordValue/random_integers-10      0.00           0.00          ~     (all equal)

Part of #133306

Release note: None

@dhartunian dhartunian requested review from a team as code owners February 3, 2025 21:33
@dhartunian dhartunian requested review from angles-n-daemons, arjunmahishi and aa-joshi and removed request for a team February 3, 2025 21:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dhartunian dhartunian force-pushed the histogram-no-windowed branch 2 times, most recently from ff9dd52 to e947cf2 Compare February 3, 2025 21:37
Previously, we used an `RWMutex` to serialize access to the windowed
histogram. This was done because there's a rotation that needs to
happen that moves the currently active histogram into the `prev`
variable and creates a fresh one to update in `cur`. This is done to
maintain the window.

This change limits the mutex usage to rotation and window
snapshotting, removing the need to take a read lock during updates
since we can atomically grab the `cur` histogram.

There's a bit of nuance to manage around the fact that `prev` can be
nil on the first iteration, since `cur` is the first histogram. After
a single rotation, `prev` will not be nil again.

The prometheus histogram itself (what's stored in the `atomic.Value`)
is thread-safe.

```
old:  507c68f Merge cockroachdb#140092
new:  ff9dd52 metric: remove mutex around windowed histogram upd
args: benchdiff "./pkg/util/metric" "-b" "-r" "BenchmarkHistogramRecordValue" "-c" "10" "--preview"

name                                     old time/op    new time/op    delta
HistogramRecordValue/insert_zero-10        30.7ns ± 8%    24.3ns ± 2%  -20.94%  (p=0.000 n=9+8)
HistogramRecordValue/insert_integers-10    37.4ns ± 6%    33.8ns ± 1%   -9.83%  (p=0.000 n=8+8)
HistogramRecordValue/random_integers-10    44.9ns ± 9%    41.7ns ± 1%   -7.25%  (p=0.000 n=8+8)

name                                     old alloc/op   new alloc/op   delta
HistogramRecordValue/insert_integers-10     0.00B          0.00B          ~     (all equal)
HistogramRecordValue/insert_zero-10         0.00B          0.00B          ~     (all equal)
HistogramRecordValue/random_integers-10     0.00B          0.00B          ~     (all equal)

name                                     old allocs/op  new allocs/op  delta
HistogramRecordValue/insert_integers-10      0.00           0.00          ~     (all equal)
HistogramRecordValue/insert_zero-10          0.00           0.00          ~     (all equal)
HistogramRecordValue/random_integers-10      0.00           0.00          ~     (all equal)
```

Part of cockroachdb#133306

Release note: None
@dhartunian dhartunian force-pushed the histogram-no-windowed branch from e947cf2 to 539487b Compare February 4, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants