[#33252] DocDB: Reduce lock contention in LongOperationTracker - #23
Open
craigsoules wants to merge 3 commits into
Open
[#33252] DocDB: Reduce lock contention in LongOperationTracker#23craigsoules wants to merge 3 commits into
craigsoules wants to merge 3 commits into
Conversation
Every `LongOperationTracker` registration went through a single process-wide mutex in `LongOperationTrackerHelper`: `Register` locked the mutex, pushed into a shared `std::priority_queue`, and issued an unconditional `notify_one` (a futex wake on nearly every registration, since the checker thread is almost always waiting). The woken checker thread immediately re-acquired the same mutex, contending with concurrent registrations. This sits on hot paths: every read query, every Raft `UpdateReplica`, log append, master heartbeat processing, and (in debug builds) every `ScopedRWOperation`. Make registration lock-free instead: - Operations are handed to the checker thread through the lock-free `MPSCQueue` from `yb/util/lockfree.h`. Registration is now one allocation plus one CAS, with no mutex and no condvar notify. - `TrackedOperation` ownership switches from `std::shared_ptr` to intrusive ref-counting (`RefCountedThreadSafe` + `scoped_refptr`) so the queue's raw pointer can carry an owned reference. The `use_count() > 1` completion check becomes `!HasOneRef()`. - The checker thread drains the intake queue into a private (and therefore uncontended) priority queue and sleeps until the next deadline, capped at 100ms so that newly registered operations are noticed. Since the minimum tracked duration in the tree is 1 second, the bounded delay in emitting a warning is not observable. Reporting is unchanged: each overdue operation still logs its own message, exact elapsed time, and stack trace. - The helper's mutex and condition variable now guard only the stop flag and are touched only by the checker thread and the destructor. - Fix `stop_` being uninitialized. - `LongOperationTracker` special members move out-of-line because `scoped_refptr` requires a complete `TrackedOperation` type at instantiation. Move assignment is implemented manually since `scoped_refptr` move assignment does not clear its source. Add a multi-threaded stress test covering concurrent registration, completion, and move semantics. Test Plan: ./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTracker ./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerStress ./yb_build.sh debug --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTracker ./yb_build.sh debug --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerStress ./yb_build.sh release daemons Assisted-By: devx/fcfd7b98-d658-403c-af84-fc430ff979e0 --- _automated - claude-fable-5 (pi)_
…rtions Follow-up to review feedback on the lock-free registration change: - Bound the intake drain to 1000 registrations per checker iteration and re-check `stop_` (now `std::atomic<bool>`) in both the drain and expired-operation loops, so that sustained high registration rates cannot starve deadline processing or block shutdown. When the drain budget is exhausted the checker skips sleeping and continues immediately. - Wake the checker thread from `Register` when the deadline is below twice the 100ms scan interval, so that runtime flags configured with sub-100ms warning thresholds do not lose the stack trace warning when the operation expires and completes between two scans. Typical durations (1s and above) still never notify. Document the timeliness guarantee in the header. - Add `LongOperationTrackerUnderLoad` test: asserts that an overdue sentinel operation is reported by the checker thread while eight producer threads keep registering, that operations completing before their deadline are never reported, and (via exact warning counts on a moved tracker) that move construction and assignment clear their source. Extract `TestLogSink` for reuse across tests. Registration throughput was compared on macOS arm64 (8 threads x 200k registrations, 60s deadlines): 4.2M/sec before the change vs 3.7-4.4M/sec after, i.e. parity within run-to-run noise on this platform. The expected production win is the elimination of the per-registration condvar notify (a futex wake on Linux) and of the mutex convoy between registering threads and the checker thread. Test Plan: ./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTracker ./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerStress ./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerUnderLoad Same three tests with the debug build type. ./yb_build.sh release daemons Assisted-By: devx/fcfd7b98-d658-403c-af84-fc430ff979e0 --- _automated - claude-fable-5 (pi)_
… test
Second round of review feedback:
- Make the short-deadline wakeup reliable instead of best-effort: the
registering thread now records `short_deadline_pending_` under the
mutex before notifying, and the checker thread waits with the
predicate overload of `wait_for`, so the wakeup cannot be lost
between a drain and the following sleep. Only deadlines below 200ms
pay for the mutex; typical registrations remain lock-free.
- Align documentation with actual guarantees: warnings are typically
logged within 100ms of the deadline, but scans can be delayed while
the checker thread is dumping stacks of other overdue operations.
Update the stale synchronization strategy comment, since `Register`
now participates in the condition variable protocol.
- Strengthen the `LongOperationTrackerUnderLoad` test:
- Register a synchronous burst of 50000 operations, well above the
checker's per-iteration drain bound of 1000, so the bounded-drain
path runs repeatedly while overdue operations are pending. The
test comment states explicitly that indefinite overload cannot be
reproduced deterministically.
- Validate move construction and move assignment independently with
separately named overdue operations, since the previous
move-and-move-back sequence structurally cancelled a hypothetical
non-clearing move constructor.
- Assert the exact form of the checker warning ("running for",
"in thread") and exactly one destructor warning per operation.
- Reduce the producer sleep from 100us to 10us for more sustained
registration pressure.
Test Plan:
./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTracker
./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerStress
./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerUnderLoad
./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerUnderLoad -n 10
Same tests with the debug build type.
./yb_build.sh release daemons
Assisted-By: devx/fcfd7b98-d658-403c-af84-fc430ff979e0
---
_automated - claude-fable-5 (pi)_
5 tasks
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.
Summary
Tracks upstream issue yugabyte/yugabyte-db#33252 (to be upstreamed from this PR).
Every
LongOperationTrackerregistration went through a single process-wide mutex inLongOperationTrackerHelper:Registerlocked the mutex, pushed into a sharedstd::priority_queue, and issued an unconditionalnotify_one(a futex wake on nearly every registration, since the checker thread is almost always waiting). The woken checker thread immediately re-acquired the same mutex, contending with concurrent registrations. This sits on hot paths: every read query, every RaftUpdateReplica, log append, master heartbeat processing, and (in debug builds) everyScopedRWOperation.Make registration lock-free instead:
MPSCQueuefromyb/util/lockfree.h. Registration is now one allocation plus one CAS, with no mutex and no condvar notify.TrackedOperationownership switches fromstd::shared_ptrto intrusive ref-counting (RefCountedThreadSafe+scoped_refptr) so the queue's raw pointer can carry an owned reference; theuse_count() > 1completion check becomes!HasOneRef().master_ts_heartbeat_long_operation_warning_ms) additionally wake the checker thread through a mutex-protected predicate, so short-deadline warnings cannot be lost; typical (1s+) registrations never touch the mutex.stop_being uninitialized.Registration throughput on macOS arm64 (8 threads x 200k registrations): 4.2M/sec before vs 3.7-4.4M/sec after, i.e. parity within run-to-run noise on that platform. The expected production win is eliminating the per-registration futex wake and the mutex convoy between registering threads and the checker thread on Linux; a Linux perf/futex comparison is still to be collected.
Test plan
./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTracker(also debug)./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerStress(new: concurrent registration/move churn; also debug)./yb_build.sh release --cxx-test debug-util-test --gtest_filter DebugUtilTest.LongOperationTrackerUnderLoad(new: overdue operations detected under sustained registration traffic and a 50k burst exercising the bounded drain; exact warning counts validate move construction/assignment clearing; also debug, and 10x repeated in release)./yb_build.sh release daemons(all call sites compile)Registeris a no-op under sanitizers, pre-existing; macOS prebuilt thirdparty lacks instrumented libc++ locally)