perf: avoid hot-path atomic write when circuit state is unchanged#30
Open
costela wants to merge 1 commit into
Open
perf: avoid hot-path atomic write when circuit state is unchanged#30costela wants to merge 1 commit into
costela wants to merge 1 commit into
Conversation
SlidingWindowBreaker.observe returns stateChangeClose on every call below threshold, so a healthy closed circuit executed openedAt.Store(0) on every single call - an unconditional atomic write to one shared cache line that is already 0. Under concurrency this bounces the cache line between cores (MESI invalidation), reintroducing the cross-core contention the lock-free design exists to avoid, for a write that changes nothing. Guard both transitions with a Load first: only open() when currently closed and only close() when currently open. A Load keeps the cache line shared across cores; the steady-state happy path becomes read-only. open()'s CompareAndSwap already no-ops when already open, but the Load avoids even issuing the read-modify-write. Benchstat (parallel benchmarks): ~9-43% faster, scaling with core count exactly as the contention hypothesis predicts; single-threaded SlidingWindow shows no change (no contention to relieve). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces cross-core contention in the circuit breaker hot path by avoiding unnecessary atomic writes to Circuit.openedAt when the circuit state is already in the desired steady state (closed or open).
Changes:
- Guard
stateObserver.Observe’sopen()/close()calls withopenedAt.Load()checks to skip redundant atomic writes. - Keep the steady-state closed-circuit path read-only when
SlidingWindowBreaker.observerepeatedly returnsstateChangeClose.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+193
to
+194
| // Only write if not already open. A Load keeps the openedAt cache line in shared state across cores, whereas | ||
| // open()'s read-modify-write would needlessly invalidate it on every hot-path call. |
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.
What
Guard the
openedAtmutation instateObserver.Observewith aLoadso a steady-state circuit doesn't write to shared state on every call.Why
SlidingWindowBreaker.observereturnsstateChangeCloseon every below-threshold call, so a healthy closed circuit ranopenedAt.Store(0)on every call — an unconditional atomic write to a cache line that's already0. Under concurrency that bounces the line between cores (MESI invalidation), reintroducing exactly the cross-core contention the lock-free design exists to avoid — for a write that changes nothing.How
stateChangeOpen→ onlyopen()ifopenedAt.Load() == 0.stateChangeClose→ onlyclose()ifopenedAt.Load() != 0.A
Loadkeeps the cache line shared across cores; the steady-state happy path becomes read-only.Results (benchstat, parallel)
Gains scale with core count, as the contention hypothesis predicts.
🤖 Generated with Claude Code