Repository navigation
Make optimistic commit lock buckets and validation policy configurable - #897
Merged
Merged
Conversation
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 9b6d038 |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces process-wide optimistic commit lock buckets (occLockBuckets) shared across all writable optimistic databases and worker threads, optimizing memory usage. Feedback on the changes suggests explicitly casting the double configuration value to uint32_t in db_settings.cpp to prevent compiler warnings, and wrapping the /proc/self/smaps_rollup read in the test fixture in a try/catch block to handle environments where the file is inaccessible.
This was referenced Oct 5, 2026
kriszyp
marked this pull request as ready for review
October 5, 2026 20:29
kriszyp
added a commit
to HarperFast/harper
that referenced
this pull request
Oct 5, 2026
TODO: set the @harperfast/rocksdb-js version in package.json and package-lock.json to the release containing HarperFast/rocksdb-js#897, #901 and #902 once it is published. The subscriber fixes this bump relies on land separately in kris/subscriber-late-commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kriszyp
force-pushed
the
fix/shared-occ-lock-buckets
branch
from
October 6, 2026 04:59
37b6c55 to
4c49e7f
Compare
cb1kenobi
approved these changes
Oct 6, 2026
Co-Authored-By: GPT-6.1 Codex <noreply@openai.com>
Co-Authored-By: GPT-6.1 Codex <noreply@openai.com>
Co-Authored-By: GPT-6.1 Codex <noreply@openai.com>
Co-Authored-By: GPT-6.1 Codex <noreply@openai.com>
Co-Authored-By: GPT-6.1 Codex <noreply@openai.com>
Drop the process-wide shared OccLockBuckets pool. Each writable optimistic
database now opens with its own pool of RocksDatabase.config({ occLockBuckets })
buckets (default 4,096) instead of RocksDB's 2^20 default, which cost about
40 MiB per database on Linux x64.
Async commits to one database already run one at a time on its commit lane, so
the only same-database bucket contention is the lane against commitSync(). A
shared pool only added contention between databases, including blocking other
databases' commits behind a write stall. The count now applies to subsequent
opens with no freeze; open databases keep theirs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Document that commitSync() callers on any thread contend with the commit lane and with each other (and lose write-group batching on a collision), count benchmark retries only inside the measurement window, reject --hot-keys below --keys, and stop surviving workers before shutdown when one fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RocksDatabase.config({ occValidation: 'parallel' | 'serial' }) selects
OptimisticTransactionDBOptions::validate_policy for optimistic databases
opened after the call (default 'parallel'). Serial validation checks
conflicts inside the write group and allocates no bucket pool.
The occLockBuckets default is 65,536 (about 2.5 MiB per database on Linux
x64) instead of RocksDB's 2^20 (about 40 MiB): async commits already run one
at a time on each database's commit lane, and 65,536 recovers the measured
64-key concurrent commitSync case.
The column-family drop suites also run under serial validation, through a
NODE_OPTIONS preload that reaches their spawned fixtures.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Document that legacy ROCKSDB_JS_COMMIT_THREAD=0 async commits contend for buckets (measured), size the 1,000-key loss at the 65,536 default, and state the parallel-validation window in which a non-transactional write is overwritten without ERR_BUSY. The benchmark gains --occ-validation, rejects non-integer sizes, and stops workers when startup fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
import() of an absolute Windows path fails with ERR_UNSUPPORTED_ESM_URL_SCHEME, which failed both worker-backed modes on every Windows leg. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
config() validated both occ fields before storing either, but still stored them before later fields (blockCacheSize, write-buffer-manager, lifecycleWaitSeconds, verificationTableEntries) were validated — a later field's throw left the occ settings applied anyway. Move both stores to the end, after every validation. Also assign OccValidationPolicy::kValidateParallel explicitly in the non-serial branch instead of relying on RocksDB's header default, so a future vendored default change can't silently flip behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Dispatch-Task: pr-maint-2d6b958413cb19eebad59492f0728393
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: fix-kriszyp_rocksdb-js_897-92a31055
#902) * Run a database's async commits on up to four concurrent commit threads The default single commit lane (#694) ran every async RocksDB commit for a database on one thread. Under load that thread saturated a core: about 38% in optimistic validation and 33% in memtable inserts, both of which RocksDB runs in parallel across concurrent writers. With four workers committing 64-key transactions (transaction-log entries on, WAL off) it committed 4.6k/s against 9.3k/s for the legacy libuv path. CommitWorker now runs up to `commitThreads` threads per database (RocksDatabase.config, default min(4, cores), read at open). A thread starts only when queued commits outnumber idle threads, so a database that is never committed to concurrently keeps one, and a burst queued before an idle thread wakes still grows the pool. Each thread runs a commit's transaction-log write and RocksDB commit back to back; `commitThreads: 1` is the previous lane. Commits stay off the libuv threadpool, so the #694 starvation fix holds. Routing log writes through the ordered txnlog lane first was measured no faster under concurrency and 13-25% slower for a caller with one commit in flight; it stays selectable with ROCKSDB_JS_COMMIT_THREAD=2. Measured on an i7-12700H, one database, txn log on, 3-5 runs: 4 workers x 64 keys 9.9k/s (lane 4.6k, legacy 9.3k); 8 workers x 1 key 202k/s (lane 108k, legacy 194k); single caller with one commit in flight unchanged. Concurrent commits finish out of order, which exposed a pre-existing race (legacy mode and commitSync() racing the lane had it too): commitFinished() paired the committed log prefix with a RocksDB sequence read before taking dataSetsMutex, so an earlier log position could commit at a later sequence in between and a flush could record a replay start past unflushed data. The sequence is now read under that mutex. Also adds the commitPipeline.commitThreads gauge, makes the transaction stress worker await every commit, extends the OCC benchmark with --log, --stat-probe and --commit-threads, and updates the occValidation / occLockBuckets guidance: serial validation gives back the concurrency gain, and large transactions need more buckets under concurrent commits. Closes #898 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Address pre-push review: ordering docs, benchmark retries, sync pin - commitSync reads the RocksDB sequence through its pinned descriptor instead of re-reading the handle's descriptor after the dataSetsMutex wait. - The descriptor's worker comment and the two-lane comment no longer promise dispatch order with more than one commit thread. - README: log position is not same-key apply order after a retried commit; consumers replaying one key must order by transaction timestamp. - The OCC benchmark retries a retryable commit on the same transaction, as db.transaction() does, so --log no longer abandons it on abort(); it also validates --commit-threads. - The lazy-start fixture allows the one-thread window a sequential awaiter can hit between a completion and the thread going idle. - Trim comment narration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Move the legacy fs-starvation demonstration out of the unit suite It asserted that a stalled legacy (libuv) commit delays fs.stat, which is a property of the runtime rather than of rocksdb-js: Deno runs node:fs outside the pool N-API async work uses, so the delay never happens there and the test failed on every Deno leg. The demonstration stays in the benchmark (--stat-probe), where legacy mode measured fs.stat p50 of 4-121 ms under load. The regression test that commits leave the threadpool free is kept, skipped on Deno, where it cannot fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Address pre-push rebase review: timestamp-ordering doc, watermark wording, bounded test wait - README: a retried commit's transaction timestamp is frozen at the same point as its log position, so replaying by timestamp does not recover true apply order either; point producers needing that at their own last-writer-wins marker instead. - DESIGN.md: every successful commit emits 'committed' on completion, not only the one that advances the watermark — out-of-order completion means an emission does not imply the emitter's own entry is visible to a committed log.query() yet. - Drop two comments that only restated the code below them. - Bound the previously-unbounded wait on the flush-correlation sampler in TransactionLogFlushedState.OutOfOrderCommitsKeepFlushCorrelationBehindUnflushedPositions so a regression that stops calling the sampler fails in 5s instead of hanging the native suite; the commit thread is joined unconditionally before the assertion so a timeout can't leave a joinable std::thread behind. Dispatch-Task: pr-maint-26ecc6f7c44020182fa8e263a375173a Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address delta review: timestamp-assignment wording, commitQueueDepth doc accuracy - README: don't claim the transaction timestamp freezes "at the same point" as the log position — it's assigned earlier, at construction/first addLogEntry, while the log position is assigned at commit-time writeBatch. Keep only the verified claim: neither value advances across a retry, so neither recovers true apply order. - docs/stats.md: commitPipeline.commitQueueDepth covers the whole commit (log write + RocksDB commit) in the default single-lane mode regardless of commitThreads; only the two-lane pipeline (ROCKSDB_JS_COMMIT_THREAD=2) splits the RocksDB commit into this queue separately from logQueueDepth. The old wording keyed the distinction off commitThreads:1, which was never the dimension that mattered. Dispatch-Task: pr-maint-26ecc6f7c44020182fa8e263a375173a Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
kriszyp
force-pushed
the
fix/shared-occ-lock-buckets
branch
from
October 6, 2026 13:31
f6a28b5 to
81c1e67
Compare
…ings config()'s trailing occLockBuckets/occValidation/commitThreads stores ran unconditionally after the verificationTableEntries read, including when that read left a pending JS exception (a throwing getter on the options object). The call still threw to JS, but the three fields were stored anyway, contradicting the adjacent "a later field's throw must still leave them untouched" comment. Return before the stores when an exception is pending. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Dispatch-Task: pr-maint-2d6b958413cb19eebad59492f0728393
The previous commit tied the guard to verificationTableEntries staying the last optional read before the OCC/commitThreads stores — true today, but a field added after it (e.g. by #767) would silently bring the leak back. Check for a pending exception once, immediately before the stores, so it holds regardless of which read is last. Also drops a comment that mischaracterized getProperty(required=true)'s absent-property status as napi_ok; it is napi_invalid_arg, same as a real type failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Dispatch-Task: pr-maint-2d6b958413cb19eebad59492f0728393
This was referenced Oct 6, 2026
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.
⊙ Problem
Every writable optimistic database opens with RocksDB's default 2^20 commit lock buckets. On Linux x64 that is about 40 MiB of mutexes per database, allocated whether the database is busy or empty. Ten empty databases measured 401 MiB of anonymous memory. Processes that open many databases (Harper opens one per schema database, plus system databases) pay that per database. Neither the bucket count nor RocksDB's other validation policy,
kValidateSerial, which needs no buckets at all, could be configured.Closes #894.
💡 Solution
Two process-wide settings, read when an optimistic database is opened. Both follow the same rules: they apply to optimistic databases opened after the call, databases already open keep what they opened with, nothing freezes, and pessimistic, read-only and secondary opens ignore them.
RocksDatabase.config({ occLockBuckets }): the size of each database's private bucket pool under parallel validation. The default is now 65,536 (about 2.5 MiB per database on Linux x64) instead of RocksDB's 2^20 (about 40 MiB). That is a behavior change: memory per optimistic database drops by about 37 MiB.commitSync()callers on the same database, or concurrent libuv commits underROCKSDB_JS_COMMIT_THREAD=0, can wait on its buckets.commitSync()case (sweep below). Workloads with 1,000-keycommitSync()mixes still lose up to about a third of their throughput and can raise the count to about 2^20.ROCKSDB_JS_COMMIT_THREAD=0mode has no lane, so its async commits contend too. With 4 workers, 65,536 buckets gave 11,474 commits/s against 12,762 at 2^20 for 64-key transactions (about −10%), and 350 against 505 for 1,000-key transactions (about −31%). Three runs each, with non-overlapping ranges. The README says so.RocksDatabase.config({ occValidation: 'parallel' | 'serial' }): maps toOptimisticTransactionDBOptions::validate_policy. The default stays'parallel'. Under'serial', RocksDB runs the conflict check inside the write group and allocates no bucket pool:OptimisticTransactionDBImpl's constructor builds one only forkValidateParallel(utilities/transactions/optimistic_transaction_db_impl.h). Measured at 0.2 MiB for three databases configured with 2^20 buckets.'serial': nearly all writes to a database are async transaction commits through the commit lane, with few concurrentcommitSync()calls or non-transactional writes on that database. It also suits large transactions and processes that open many databases.Harper will set
occValidation: 'serial'(and its own bucket count) in a companion Harper PR. rocksdb-js keeps a general-purpose default.⚖️ Alternatives
shared_lock_buckets, this PR's first revision). This is removed. Buckets are held throughDB::Write(), so commits in unrelated databases contend on hash collisions, and a database in a write stall blocks every database whose keys collide with its pending commit. Measured with four databases and one worker each: −40% commits/s at 1,000 keys per transaction and −58% at 4,096 keys, against private pools.commitSync()mixes lost about 40% of their throughput.🔧 Changes
src/binding/database/db_descriptor.cpp: passOptimisticTransactionDBOptions{ occ_lock_buckets, validate_policy }toOptimisticTransactionDB::Open.validate_policyis now assigned explicitly in both branches (serial or parallel) rather than relying on RocksDB's header default for the parallel case, so a future vendored default change can't silently flip behavior.src/binding/database/db_settings.{h,cpp}: atomic settings for the bucket count (default 65,536) and the validation policy (default parallel).config()validates both before applying either:occLockBucketsmust be an integer in 16..16,777,216, andoccValidationmust be the string'parallel'or'serial'(TypeErrorfor a non-string,RangeErrorfor any other string). The shared pool, its mutex and the freeze are removed. The two stores are now applied last, after every other field's validation, so a later field's thrown error (negativeblockCacheSize/writeBufferManagerSize, invalidlifecycleWaitSeconds, a secondverificationTableEntrieschange) leaves the occ settings untouched.src/load-binding.ts,README.md: document both settings, their memory cost, and when to use each. The README also states a window that main already has under parallel validation: a non-transactionalputSync()takes no bucket, so one that lands between a commit's conflict check and its write is overwritten withoutERR_BUSY.AGENTS.mdinvariant 24: under serial validation, the write-group check also refuses a dropped family. It is defense in depth only, because the claim protocol still applies under both policies and is the only protection for pessimistic commits.src/binding/database/commit_worker.h: correct the comment that said optimistic validation does not block. Parallel validation takes bucket mutexes, sorted acquisition keeps that deadlock-free, and with private pools onlycommitSync()callers contend with the lane.src/binding/database/DESIGN.md,DESIGN.md: record both policies, why the default pool is small, and why the pool must stay private.benchmark/occ-commit-contention.mts: a standalone benchmark (same convention asbenchmark/mutex-contention.mts). It covers one database with many workers, four databases, async pluscommitSync(), real conflicts, and the memory of ten empty databases.--libcompares builds, and--occ-lock-bucketsand--occ-validationset the two configs.✅ Verification
test/occ-lock-buckets.test.ts(child-process fixturetest/fixtures/fork-occ-lock-buckets.mts):commitSync()counter loses no updates, under both policies;/proc/self/smaps_rollupis unreadable or lacks anAnonymous:line — reviewer request):import()of an absolute Windows path failed on every Windows leg.test/occ-serial-drop.test.tsreruns Defer physical column-family drops behind admitted commits #850's suites (drop-deferred-reclamation,drop,txn-close-commit-uaf; 60 tests) underoccValidation: 'serial'. ANODE_OPTIONS=--importpreload reaches the suites' spawned fixtures. They pass on macOS and Linux with unchanged expectations and error messages.Openoverload restored, the default, configured and serial memory tests fail (401, 120 and 120 MiB against bounds of 60, 30 and 30).occValidation: 'serial'reaches RocksDB's serial path (DBImpl::WriteWithCallback), and'parallel'reachesCommitWithParallelValidate.pnpm check,pnpm test(76 files, 1,108 passed, 10 skipped) andpnpm test:native(299 passed). Linux: the two OCC test files. The full Linux, Windows, Bun and Deno runs are left to CI.main(2026-10-06): resolved aDESIGN.mdadd/add conflict across the 13 replayed commits (merged both PRs' index entries; kept this branch's own later wording revisions for thesrc/binding/database/DESIGN.mdline). Post-rebase: full production build,pnpm check, the full JS suite (1,124 passed, 10 skipped), native GoogleTest (309 passed) and stress tests (9 passed) all green on Linux. A second, delta pre-push review round on the rebased head retired the config-atomicity finding (now fixed, see 🔧 Changes) and found no new actionable issues; the remaining open findings are the pre-existing ones already covered above and in ⚖️ Alternatives (default-bucket-count tradeoff, Linux-only memory assertions, Bun/Deno serial-drop skip, standalone benchmark not wired intopnpm bench).main(2026-10-06, same day):mainhad advanced 13 more commits; this branch's own tip had also grown by one — #902, a PR based on this branch rather thanmain, had merged into it directly. SameDESIGN.mdadd/add conflict pattern, now hit 4 times across the 16 replayed commits; resolved the same way each time (merge both sides' index lines).git range-diffagainst the old head confirmed every commit replayed cleanly with no silent content changes outside those hunks. Full build,pnpm check, the full JS suite (1,151 passed, 10 skipped), native GoogleTest (315 passed) and stress tests (9 passed) all green on Linux. Two further delta pre-push review rounds found one genuine, PR-scoped bug the two prior rounds missed: a throwing getter onverificationTableEntries(not just a wrong type) left a pending exception the atomicity fix above didn't check, soconfig()threw to the caller but still silently appliedoccLockBuckets/occValidation/commitThreads(see the 🔧 ChangesYour callnote). Fixed, and empirically verified: a database opened right after the throwing call and burst with 50 commits now reaches the real default thread ceiling rather than the leaked value. Verdict moved from CHANGES to COMMENTS; converged on the same pre-existing/previously-adjudicated findings as the first rebase round.disableWAL: trueunless noted.commitSync()workers, 64 keys, async commits/s: 4,096 → 2,913, 16,384 → 3,695, 65,536 → 4,733, 2^20 → 4,999 (main: 4,948). At 1,000 keys only 2^20 recovers. The 2^20 run of this build matches main, so the code itself adds no overhead.Stop verification-table slot collisions from answering FRESH for another key (#901) fixed the pre-existing VT collision bug behind this PR's intermittent
verification-table.test.tsfailure, and has merged. Run a database's async commits on up to four concurrent commit threads (#902) was based on this PR's branch (notmain) and has since merged into it — its commit is now this branch's own tip, so the single-lane contention numbers above are superseded by that PR's own measurements wherever the two disagree. A Harper companion PR (to come) setsoccValidation: 'serial'.Related PRs: #901 independent, #902 overlaps (merged into this branch), #890 independent, #778 independent, #742 independent, #767 independent, #842 independent, #900 independent, #117 independent, #88 independent
Complexity: low
🤖 Generated with Claude Code
Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=12; full=5 @ 9ed9cad
Review-Attention: study ~20m (critical: transaction.cpp, transaction_log_store.cpp +2; decisions: default-commit-threads, occ-bucket-default, settings-read-at-open, expose-serial-policy, do-less-alternative) @ 9ed9cad