feat: add getPinned returning a sealed PinnedResult - #48
Conversation
Two corrections and a benchmark retraction1. The claim that the C layer has no fixed-capacity destination is out of date. The description above and the first commit message both say truncation is an artifact the Java binding invented, citing /* Returns 1 if value fits in buffer, 0 if buffer too small.
Sets *vallen to actual value size.
If buffer is too small, no data is copied but *vallen is set. */
unsigned char rocksdb_get_into_buffer(..., char* buffer, size_t buffer_size,
size_t* vallen, unsigned char* found, char** errptr);That is 2. The documented C++ pattern is one caller-owned slice reused across gets with 3. Retracting every benchmark number in this PR. All JMH runs were contaminated — the machine was concurrently running a game at ~30% CPU, Specifically retracted: the claimed 1.4–3.5% regression from wrapper allocations, the 18.5% drop on The follow-up commit (44bb587) that those numbers motivated still stands on its own terms — it strictly removes allocations from every copying path, collapses two confined arenas per pinned read into one, and drops an intermediate slice object — but it is labelled unverified, and no performance claim should go in the README until this is re-run on an idle machine.
|
9791bf7 to
6004c89
Compare
Phase 1 of #47, additive: no existing signature changes. The read path encoded three outcomes into one number -- a length, -1 for not-found, and, unrepresentably, "your buffer was too small". Probed against a real DB with a 32-byte value and an 8-byte destination: [buffer] returned=32 copied=8 content=ABCDEFGH [segment] returned=32 byteSize=8 content=ABCDEFGH [segment] present-but-empty=0 absent=0 The ByteBuffer tier was worse than the issue described: it advanced position by the truncated count, leaving the buffer in exactly the state a successful 8-byte read would, so the idiomatic get-then-flip returned a clean-looking buffer with 24 bytes silently gone. The third line is #44 -- absent and present-but-empty are indistinguishable on the segment tier. getPinned replaces that with a sealed type on the pinned path: try (PinnedResult result = db.getPinned(key)) { switch (result) { case PinnedResult.Found found -> consume(found.value()); case PinnedResult.NotFound ignored -> handleMiss(); } } The length is intrinsic -- found.value() is a MemorySegment whose byteSize() is the value length, and there is no caller-supplied capacity, so insufficient capacity is not a representable state. NotFound carries no length, so it cannot be read as a present-but-empty value; that is the #44 gap, closed on this path. The borrowed segment is scoped to the result, so touching it after the try-with-resources throws IllegalStateException instead of reading freed memory. The result is AutoCloseable rather than taking an Arena. A Found holds its block-cache entry pinned until released, and an Arena parameter makes it easy to hand in a long-lived scope, which accumulates pins and surfaces as native memory growth rather than a heap leak -- the hardest kind to attribute. Statement scope is the safe default. 40 overloads across ReadWriteDB, ReadOnlyDB, BlobDB, SecondaryDB, TtlDB, TransactionDB, OptimisticTransactionDB and Transaction: every key tier (byte[], ByteBuffer, MemorySegment), ReadOptions variants, and column-family variants wherever the type has them. One new symbol, rocksdb_transactiondb_get_pinned; TransactionDB's non-CF get used the malloc-returning rocksdb_transactiondb_get and had no pinned form. Also fixes a standing rule violation: rocksdb_pinnableslice_value and _destroy were each mapped three times -- RocksDB, Transaction, TransactionDB -- against CLAUDE.md's no-duplicate-symbol rule, which names PinnableSlice as the wrapper to build for exactly this. The copying get helpers do not go through that wrapper. Doing so allocated a PinnableSlice and a PinnedResult.Found per read on the hottest path in the library, where the previous code allocated nothing, so they instead use two raw-pointer statics -- PinnableSlice.valueOf and .destroy -- which share the class's method handles. The symbols stay mapped once and nothing is allocated. copyInto also uses the offset form of MemorySegment.copy rather than asSlice + copyFrom, avoiding an intermediate slice object. A pinned read allocates one confined Arena rather than two: the arena opened for the error holder is handed to the PinnableSlice, which owns it and closes it in tryClose, before the native destroy so a use-after-close throws rather than reading memory about to be freed. On reusing one PinnableSlice across many gets The C++ API documents exactly that, in "PinnableSlice; less memcpy with point lookups" (RocksDB blog, 24 August 2017): https://rocksdb.org/blog/2017/08/24/pinnableslice.html PinnableSlice pinnable_val; while (!stopped) { auto s = db->Get(opt, cf, key, &pinnable_val); // ... use it pinnable_val.Reset(); // then release it immediately } That pattern is not reachable from the C API this project is pinned to. Every declaration in c.h mentioning rocksdb_pinnableslice_t either returns one or takes one to read or destroy; there is no rocksdb_pinnableslice_create, and nothing accepts a caller-supplied slice to fill. rocksdb_get_pinned does `new (rocksdb_pinnableslice_t)` on every call (db/c.cc). rocksdb_get_pinned_v2 appears to address this -- its header comment claims it avoids unnecessary allocations -- but its body is byte-for-byte identical, differing only in struct name. So the native new/delete pair happens once per get whatever shape the Java API takes. Reuse could only save the Java side: the wrapper, the Found record, the scoping Arena, the length holder. PinnedReuseBenchmark measures that ceiling instead of guessing -- perGet is the public API, reused hoists all scratch out of the loop so nothing is allocated per get, parameterized on reads per invocation (1, 2, 8, 32) and value size. Note what reused gives up to get there: its segment is unscoped, so it cannot detect use after destroy -- the protection #45 exists to add. Two package-private hooks, RocksDB.getPinnedRaw and PinnableSlice.valueInto, make this measurable without widening the public API; delete them if the answer is that reuse buys nothing. Documentation README claimed "a failure is always loud", which the copy-into overloads contradict. Now stated outright and linked to #47. CLAUDE.md's three-tier rule gained a note that on the read path the tiers describe the key only, with an explicit instruction not to add new copy-into overloads, since #47 warns the next contributor would otherwise restore them. scripts/benchmark.sh no longer runs `mvn install`, which CLAUDE.md forbids. One reactor invocation resolves sibling modules to their target/classes, so nothing needs installing; it also uses ./mvnw rather than mvn, drops the log-scraping of the classpath, and takes an optional class argument. Prior art, for phases 3 and 4 rocksdbjni truncates identically in C++ (java/rocksjni/kv_helper.h, both Fetch() implementations) but clamps the destination's limit instead of advancing position, and documents it. Upstream's own TODO at java/src/main/java/org/rocksdb/RocksDB.java:1876 regrets the int return. The C++ API has no fixed-capacity destination at all -- include/rocksdb/db.h:599-640 offers std::string*, which grows, and PinnableSlice*, which borrows -- so truncation is an artifact the Java binding introduced. The C API, however, has since grown one and got it right (c.h:3583): /* Returns 1 if value fits in buffer, 0 if buffer too small. Sets *vallen to actual value size. If buffer is too small, no data is copied but *vallen is set. */ unsigned char rocksdb_get_into_buffer(...); That is CopyResult in C, including the no-partial-copy behavior #47 recommends, plus a `found` out-param that closes #44 -- and its PinnableSlice is a stack local, so it avoids the per-call new/delete that rocksdb_get_pinned forces. Phase 3 should be built on it rather than hand-rolled. Pin lifetime: shared scope, unconditional release The arena scoping the borrowed segment is created lazily inside PinnableSlice and is Arena.ofShared(), not confined. A confined arena can only be closed by its creating thread, and tryClose runs inside NativeObject.close(), which swallows what it throws -- so closing a result from another thread silently skipped rocksdb_pinnableslice_destroy while the pointer had already been nulled, stranding the block-cache entry with nothing able to retry. Making the destroy unconditional via finally fixed the leak but left the segment readable over released memory, which is the use-after-free class #45 exists to prevent. A shared arena closes from any thread, so "use after close throws" holds no matter who closes. This reverts an earlier optimization that had getPinned hand its own confined arena to the slice to save one allocation per pinned read. That arena outliving the call was what made the confinement observable. The error holder and key go back to a promptly closed confined arena, and callers who never touch the bytes never allocate the scope at all. PinnedLeakProbeTest pins the behavior: close from a second thread, then assert the borrowed segment is dead -- which is only true if the arena closed, which is only true if the destroy ran. MEASUREMENT STATUS: unverified. The allocation changes are defensible on first principles -- strictly fewer allocations on every path -- but every JMH run taken while preparing this was contaminated: the machine was concurrently running a game at ~30% CPU with load average up to 5.8. A three-fork run came back 6x slower across the board than single-fork runs of the same benchmarks minutes earlier. So the numbers that motivated the allocation work (a claimed 1.4-3.5% regression, an 18.5% drop on readsMemorySegment) are not trustworthy, and neither is the 2x figure for pinned reads on 64 KiB values. No performance claim goes in the README until this is re-run on an idle machine: ./scripts/benchmark.sh PinnedReadBenchmark ./scripts/benchmark.sh PinnedReuseBenchmark Testing 22 new tests (GetPinnedTest, PinnedLeakProbeTest): found/not-found, the empty-vs-absent distinction, exhaustive switch, intrinsic length, use-after-close, idempotent close, all three key tiers, snapshot ReadOptions, CF reads including across a reopen, and smoke coverage on every DB type. 357 -> 379 tests, 0 failures. Javadoc clean. Phase 1 is additive, so the truncating overloads remain and still truncate. Phases 3 and 4 are breaking and belong in one release; #47 stays open for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6004c89 to
9d12351
Compare
|
this requires facebook/rocksdb#14984 to avoid creating a new |
|
found a better way in #57 |
Phase 1 of #47 — additive and non-breaking. No existing signature changes.
The problem, reproduced
Probed against a real DB, 32-byte value into an 8-byte destination:
The ByteBuffer tier is worse than #47 describes: it advances
positionby the truncated count, so the buffer ends in exactly the state a successful 8-byte read would leave it in — the idiomaticdb.get(k, buf); buf.flip();hands back a clean-looking 8-byte buffer with 24 bytes silently gone. The third line is #44.The shape
byteSize()is the value length, and there is no caller-supplied capacity, so insufficient capacity is not a representable state.NotFoundcarries no length, closing get(MemorySegment, MemorySegment) cannot distinguish "key absent" from "value is empty" #44's gap on this path.IllegalStateExceptionrather than reading freed memory.AutoCloseablerather than anArenaparameter: aFoundpins its block-cache entry until released, and anArenamakes it easy to hand in a long-lived scope, which accumulates pins and presents as native memory growth rather than a heap leak.Surface
40 overloads across
ReadWriteDB,ReadOnlyDB,BlobDB,SecondaryDB,TtlDB,TransactionDB,OptimisticTransactionDBandTransaction— every key tier,ReadOptionsvariants, CF variants wherever the type has them. One new symbol,rocksdb_transactiondb_get_pinned.Rule violation fixed en route
rocksdb_pinnableslice_value/_destroywere each mapped three times (RocksDB,Transaction,TransactionDB), against CLAUDE.md's no-duplicate-symbol rule — which namesPinnableSliceas the wrapper to build for exactly this.The copying helpers deliberately do not go through that wrapper: doing so allocated a
PinnableSlice+ aPinnedResult.Foundper read on the hottest path. They use raw-pointer statics that share the same method handles — symbols mapped once, nothing allocated. A pinned read also now opens one confinedArenainstead of two.On reusing one
PinnableSliceacross many getsThe C++ API documents exactly that, in PinnableSlice; less memcpy with point lookups (RocksDB blog, 24 Aug 2017):
That pattern is not reachable from the C API. Every
rocksdb_pinnableslice_tdeclaration inc.heither returns one or takes one to read/destroy; there is norocksdb_pinnableslice_create, and nothing accepts a caller-supplied slice to fill.rocksdb_get_pinneddoesnew (rocksdb_pinnableslice_t)per call.rocksdb_get_pinned_v2claims in its header comment to avoid unnecessary allocations, but its body is byte-for-byte identical.So reuse could only save the Java side.
PinnedReuseBenchmarkmeasures that ceiling —perGetvs everything hoisted out of the loop — parameterized on reads per invocation (1, 2, 8, 32) and value size. What the hoisted variant gives up is the segment's scope, i.e. the protection #45 exists to add.Correction to prior art
The C++ API has no fixed-capacity destination (
db.h:599-640:std::string*grows,PinnableSlice*borrows), so truncation is an artifact the Java binding introduced. The C API, however, has since grown one and got it right (c.h:3583):That is
CopyResultin C — including the no-partial-copy behavior #47 recommends and afoundout-param that closes #44 — and itsPinnableSliceis a stack local, avoiding the per-callnew/delete. Phase 3 should be built on it. Details: #47 (comment)Measurement status: unverified
The allocation changes are defensible on first principles, but every JMH run taken while preparing this was contaminated — a game at ~30% CPU, load average up to 5.8, and a three-fork run 6× slower across the board than single-fork runs minutes earlier.
Retracted: the claimed 1.4–3.5% wrapper-allocation regression, the 18.5% drop on
readsMemorySegment, and the 2× advantage for pinned reads on 64 KiB values. No performance claim goes in the README until re-run on an idle machine:scripts/benchmark.shno longer runsmvn install(CLAUDE.md forbids it), uses./mvnw, and takes an optional class argument.Testing
21 new tests in
GetPinnedTest. 357 → 378 tests, 0 failures. Javadoc clean.Not in scope
Phase 1 is additive, so the truncating overloads remain and still truncate. Phases 3 and 4 are breaking and belong in one release; #47 stays open.
🤖 Generated with Claude Code