Skip to content

[#33252] DocDB: Reduce lock contention in LongOperationTracker - #23

Open
craigsoules wants to merge 3 commits into
masterfrom
craig/long-operation-tracker-lock-contention
Open

[#33252] DocDB: Reduce lock contention in LongOperationTracker#23
craigsoules wants to merge 3 commits into
masterfrom
craig/long-operation-tracker-lock-contention

Conversation

@craigsoules

Copy link
Copy Markdown

Summary

Tracks upstream issue yugabyte/yugabyte-db#33252 (to be upstreamed from this PR).

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 (bounded to 1000 per iteration so deadline processing and shutdown are never starved) into a private, uncontended priority queue and sleeps until the next deadline, capped at 100ms. Reporting is unchanged: each overdue operation still logs its own message, exact elapsed time, and stack trace.
  • Deadlines shorter than 200ms (possible via runtime flags such as 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.
  • Fix 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)
  • Linux sanitizer (ASAN/TSAN) runs via CI (Register is a no-op under sanitizers, pre-existing; macOS prebuilt thirdparty lacks instrumented libc++ locally)

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)_
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.

1 participant