Repository navigation
refactor(diagnostics): Guard diagnostic counters with a lock instead of a queue - #539
Open
abelonogov-ld wants to merge 1 commit into
Open
abelonogov-ld wants to merge 1 commit into
abelonogov-ld wants to merge 1 commit into
Conversation
…of a queue Every dropped event and every delivered batch goes through DiagnosticCache, and a DispatchQueue.sync for a counter increment costs more than the increment. Adds UnfairLock.withLock, which inlines to the same code as lock()/unlock(). Co-authored-by: Cursor <cursoragent@cursor.com>
This branch has not been deployed
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
1 participant
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.
Summary
DiagnosticCacheserialized its counters with a private serialDispatchQueueandsync. Every dropped event (incrementDroppedEventCount) and every delivered batch (recordEventsInLastBatch) goes through it, on the caller's thread. With the event-durability work, a full event store counts every refused evaluation here, so a queue hop per counter increment is visible at that rate.This replaces the queue with the existing
UnfairLock(os_unfair_lockon Darwin,NSLockelsewhere), and addsUnfairLock.withLock(_:).Why a closure-based
withLockis freewithLockis@inline(__always)and takes a non-escaping closure. With-O -wmo, it compiles to the same assembly as callinglock()/unlock()by hand: no closure context allocation and no retain/release. Only-Ononebuilds pay for a real call.On non-Darwin platforms,
UnfairLockisNSLock, and the matchingwithLockis declared onNSLock. That way it doesn't depend on Foundation versions that shipNSLocking.withLock.Behavior
There is no behavior change: the same state and the same reset semantics, only a different mutual exclusion primitive.
Testing
The full iOS suite passes (684 tests).
Note
Overview
DiagnosticCacheno longer serializes in-memory diagnostic counters through a private serialDispatchQueueandsync. It now guards the same fields withUnfairLockand a newwithLockhelper, motivated by high-frequencyincrementDroppedEventCountcalls when the event store is full.UnfairLockgains an@inline(__always) withLockon Darwin; non-Darwin builds get the same API via anNSLockextension so callers do not depend on newer FoundationNSLocking.withLock. Documented intent is unchanged reset and counter semantics—only a cheaper mutual-exclusion primitive on the evaluation thread.Reviewed by Cursor Bugbot for commit 60601c1. Bugbot is set up for automated code reviews on this repo. Configure here.