Dedicated per-database commit thread for async transaction commits#694
Draft
kriszyp wants to merge 3 commits into
Draft
Dedicated per-database commit thread for async transaction commits#694kriszyp wants to merge 3 commits into
kriszyp wants to merge 3 commits into
Conversation
Async Transaction.commit() no longer posts one napi_create_async_work item (libuv threadpool slot) per commit. Each database's DBDescriptor owns a dedicated commit thread that runs the txn-log writeBatch + RocksDB commit + commitFinished in dispatch order, so slow commits (write stalls, large batches, txn-log contention) cannot occupy libuv slots and starve unrelated async work (fs, dns, crypto, async gets). Measured: fs.stat p50 during a heavy write burst 15.1ms -> 0.12ms at pool=4, with commit throughput at parity or better in every leg. ROCKSDB_JS_COMMIT_THREAD selects the mode: 0/false = legacy libuv path (kill switch, same build A/B), default = single lane, 2 = experimental two-lane txnlog->commit pipeline (kept selectable for real-workload evaluation; slower than single lane on synthetic loads). The commit path is split into executeLogWork/executeCommitWork stages shared by all modes, and the worker drains its whole queue per wakeup (future aggregated-writev hook). Completions are marshalled to the originating env via per-env tsfns held on the descriptor under commitMutex; the commit thread calls them under that mutex and a dying env's tsfn is released from the module env-cleanup hook (DBRegistry::ReleaseCommitCompletionsByEnv), the same env-teardown discipline as EventEmitter::notify (a per-commit tsfn acquire is not sufficient - env teardown does not honor acquire counts). Regression test: test/commit-teardown.test.ts with the ROCKSDB_JS_COMMIT_DELAY_MS seam; crashes 3/3 without the cleanup-hook release. Also adds commitPipeline.logQueueDepth/commitQueueDepth gauges to db.getStats(), named commit threads (rocksdb-commit, rocksdb-txnlog) via a new setThreadName platform helper, and docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Env-gone drop paths (worker terminate with commits in flight) now close the transaction handle so the shared descriptor does not accumulate orphaned RocksDB transactions until database close; close() is cross-thread safe (skips napi_delete_reference off the owning thread). - executeCommitWork fails the commit (Aborted) instead of silently resolving success when the handle/descriptor is torn out between the stages (reachable via DBHandle::close()'s timed-out async-work wait). - registerCommitCompletion refuses (commitCompletionsClosed, set under commitMutex by finishClose's release pass) once the descriptor's completion plumbing has shut down; a commit racing another env's close falls back to the legacy libuv path instead of re-creating a tsfn that would never be released (which pinned that env's event loop forever). - DestroyDB now claims the descriptor (beginClose) and leaves the registry entry in place until finishClose completes, mirroring CloseDB, so the env-cleanup hooks can always reach the descriptor and a concurrent OpenDB waits instead of re-opening a path whose files are being destroyed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit 22c78bc |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated, per-database commit pipeline (CommitWorker) for async transaction commits to prevent libuv threadpool starvation, including an experimental two-lane pipeline mode and robust lifecycle management for thread-safe function (tsfn) completions during environment teardown. The review feedback suggests truncating thread names to 15 characters on Linux to prevent pthread_setname_np from failing with ERANGE, and using 'bun' in process.versions instead of direct property access to avoid TypeScript compilation errors under strict type checking.
…nux thread names - The teardown test's 40 rounds x 2 iterations exceed the 120s timeout on macOS/Windows CI runners (slow worker spawns + the delayed commit backlog each round leaves on the shared commit thread). Scale rounds/iterations by platform like the notify-teardown test does. - pthread_setname_np on Linux fails with ERANGE (name not set at all) for names over 15 chars; truncate in setThreadName so long names still stick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Async
Transaction.commit()no longer posts anapi_create_async_workitem tothe libuv threadpool for each commit. Instead, every database gets a dedicated
commit thread (owned by its
DBDescriptor) that runs the txn-logwriteBatchCommit()+commitFinished, and marshals completion back to theoriginating env via a per-env threadsafe function.
Each async commit used to hold one of the (default 4) libuv slots for the entire
commit — slots shared with async
get()on block-cache miss,fs,dns, andcrypto. Under slow commits (write stalls, large values/blobs, txn-logcontention) a write burst pins all the slots and starves exactly that unrelated
async work — the degraded-mode behavior seen in real incidents. Moving commits
onto a dedicated per-DB thread removes that coupling categorically.
Modes (
ROCKSDB_JS_COMMIT_THREAD)log write and RocksDB commit back to back per transaction, in dispatch order.
0/false— legacy: the previous libuv async-work path (kill switch;same build gives a clean A/B).
2— two-lane pipeline: a transaction-log lane writes the log batch,then forwards to the commit lane, letting the stages overlap across
transactions. Measured slower than single-lane on synthetic loads (the
per-txn handoff outweighs the overlap for small commits — see benchmarks),
but kept selectable for evaluation on real workloads, where per-lane queue
depths also show which stage bottlenecks.
The commit path is split into
executeLogWork/executeCommitWorkstagesshared by all modes, and the worker drains its whole queue per wakeup — the
structural hook for a future aggregated-writev of pending log batches (in
either mode).
Why one thread per database (not a pool)
logSequenceNumberand records the RocksDB seq in commit order).writeMutexfor free.WAL-fsync'd loads (
WriteOptions.syncis never set → false).locks are taken at put time; optimistic validation doesn't block).
Env-teardown safety
The commit thread is owned by the shared descriptor and outlives any single env,
so a worker env can be torn down (
worker.terminate()) while its commits arestill queued/executing. Completion tsfns are therefore held on the descriptor,
keyed by env, under
commitMutex, and a dying env's tsfn is released from themodule env-cleanup hook (
DBRegistry::ReleaseCommitCompletionsByEnv) — whichNode runs before it frees that env's tsfns. The commit thread calls the tsfn
under the same mutex, so it either delivers safely or observes the entry gone
and drops the completion. This is the same discipline
EventEmitter::notifyuses for HarperFast/harper#1370; a per-commit
napi_acquire_threadsafe_functiondoes not close the window (env teardown doesn't honor the acquire count).
Regression test:
test/commit-teardown.test.ts(worker fires in-flight commits,parent terminates it mid-flight). A
ROCKSDB_JS_COMMIT_DELAY_MSseam widens thewindow so it reproduces deterministically. Verified load-bearing: disabling only
the cleanup-hook release aborts/segfaults every run; restored, it is clean.
Observability
Two new gauges in
db.getStats()/db.getStat()(documented indocs/stats.md):commitPipeline.commitQueueDepthandcommitPipeline.logQueueDepth. Commit threads are named (rocksdb-commit,rocksdb-txnlog) via a newsetThreadNameplatform helper.Benchmarks (same build A/B/C, pool=4,
withLog)Concurrent commit throughput (commits/s):
=0)=2)Heavy starvation leg (K=16 chains, 200 puts × 4 KB + txn log, probes during the
burst):
Small serial commit breakdown: async 1-put 64.5k → 74.7k/s (+16%), with
txn-log 54.1k → 62.3k/s (+15%);
commitSyncand directputSyncunchanged.The two-lane numbers are why single-lane is the default: the per-txn cross-lane
handoff (~3–5 µs mutex + wakeup) exceeds the log∥commit overlap for small
commits, and heavy commits showed no overlap win either.
Tests
ROCKSDB_JS_COMMIT_THREAD=0,and
ROCKSDB_JS_COMMIT_THREAD=2.pnpm test:stress,pnpm check(type/lint/fmt): green.test/commit-teardown.test.ts(worker env teardown with in-flightcommits, all modes).
Where to focus review
The TSFN lifecycle and close/teardown ordering are the risk surface:
DBDescriptor::{register,dispatch,finish,release}CommitCompletion+finishClose()+ thebinding.cppenv-cleanup hook, andCommitWorker::enqueue/shutdown. A cross-model review (Codex + Claude domainpass; Gemini leg failed to run) already drove four fixes now in the diff:
env-gone drop paths close the txn handle (no orphaned
rocksdb::Transactionaccumulation on the shared descriptor),
executeCommitWorkfails the commitinstead of silently resolving success if the handle is torn out mid-pipeline,
a
commitCompletionsClosedflag stops a commit racingfinishClosefromre-creating an unreleasable tsfn (falls back to the legacy path), and
DestroyDBnow closes before unlinking the registry entry (CloseDB'sdiscipline) so env-cleanup hooks can always find the descriptor.
Open (known, deliberately out of scope):
OpenDB's wait predicate resetsentry.descriptorwhile aclose is in flight, so an env-cleanup hook in exactly that window can miss
the descriptor (backstopped by
finishClose's release pass, which runsafter the commit thread is joined). Shared with the pre-existing listener
cleanup path — follow-up issue rather than this PR.
commitFinishedfinalizes the txn-log entry when RocksDB
Commit()returns a hard non-Busyerror — replication-relevant, filed as a follow-up issue.
Strained-database validation
The tables above are from an unstrained DB where the JS thread is the
bottleneck (per-thread CPU: JS ~101% in every mode, commit thread ≤71%). The
mode ranking was therefore re-validated on a 46 GB database (9.3M keys,
mixed 1–8 KB values) under sustained load that accumulates real compaction
debt during measurement (1–2.6 GB pending compaction, up to 8 concurrent
compactions, tens of seconds of cumulative write-stall time; JS thread drops
to ~50% — the native side becomes the bottleneck). 120 s legs, P=16, pool=4,
identical pristine DB copy per leg:
=0)=2)The single-lane default wins throughput in both regimes, and the starvation
elimination holds on real strain — during write-stall windows legacy commits
pin the libuv pool (fs.stat p99 ~50 ms) while the dedicated thread leaves it
untouched. Two-lane loses in both regimes (the per-txn handoff compounds
under load); it stays selectable for real-workload evaluation but the
synthetic evidence is consistently against it.
Context
Root-cause fix for Harper write-path libuv starvation. Related follow-up
issues: #691 (dedicated read thread pool), #692 (RocksDB Merge operator for
commutative increments). A stacked PR adds
commitMany(batched JS↔nativebarrier, full per-txn isolation preserved).
Library-repo docs updated in-tree (
docs/stats.md,AGENTS.md); noHarperFast/documentation PR needed (no Harper-facing surface).
Generated by an LLM (Claude Fable 5), driven and reviewed by Kris.
🤖 Generated with Claude Code