feat: scoped zero-copy get(key, Mapper) across every DB type + RocksIterator - #57
feat: scoped zero-copy get(key, Mapper) across every DB type + RocksIterator#57dfa1 wants to merge 1 commit into
Conversation
Finishes #55/#57. withPinned is now get(key, Mapper<R>) — a fourth tier alongside byte[]/ByteBuffer/MemorySegment, present everywhere those already are: ReadWriteDB, ReadOnlyDB, TtlDB, SecondaryDB, BlobDB, OptimisticTransactionDB, TransactionDB, Transaction, and (as key(Mapper<R>)/value(Mapper<R>), since iterators have no get()) RocksIterator. Two native mechanisms, one caller-facing shape: - rocksdb_t*-backed wrappers (ReadWriteDB, ReadOnlyDB, TtlDB, SecondaryDB, BlobDB, OptimisticTransactionDB via its baseDb) reuse RocksDB.withPinned/withPinnedCf against rocksdb_get_pinned_v2/_cf_v2, exactly as already built for ReadWriteDB. - TransactionDB and Transaction have no _v2/pinnable_handle_t equivalent in c.h — only the older rocksdb_pinnableslice_t API (rocksdb_transactiondb_get_pinned(_cf), rocksdb_transaction_get_pinned (_cf)). Added a second shared core, RocksDB.withPinnableSlice, with the identical arena-before-destroy ordering but built on rocksdb_pinnableslice_value/destroy instead. Each caller passes in its own already-bound value/destroy MethodHandles rather than re-mapping the symbols centrally. - RocksIterator has neither: rocksdb_iter_key/value hand back a pointer into a buffer the iterator reuses in place on the next seek/next/prev, with no separate destroy step at all. key(Mapper)/ value(Mapper) still bind the view to a confined Arena that closes the moment the callback returns — not because the memory would be freed (it isn't), but because it turns the exact bug an earlier commit (b7c8ba1, "scope iterator segments to the current position") fixed for keySegment()/valueSegment() into a structural impossibility instead of a documented caveat: a view can no longer silently start reporting a different position's bytes after next()/prev() runs, because the arena that backs it no longer exists. Two API changes on top of the expansion, both requested mid-review: - PinnedReader<R> is now Mapper<R>, with one method, R map(MemorySegment). - Mapper.map no longer declares throws Exception. That was there only to let a caller's deserialization logic propagate checked exceptions; dropping it cascades through every get/key/value overload built on it, so none of them declare throws Exception either — the same Function<T,R>-shaped contract as the rest of java.util.function. Checked exceptions a callback body still needs (e.g. Thread#join's InterruptedException in a test) now have to be caught inside the lambda, same as implementing any other checked-exception-free functional interface. Test coverage: WithPinnedTest renamed to PinnedGetTest (its own method names lost the "pinnedReader" segment too, now just "zeroCopy" where the type name would have leaked into the test name); a new BlobDBTest since none existed on this branch; happy-path/absent-key/CF coverage added to ReadOnlyDBTest, TtlDBTest, SecondaryDBTest, OptimisticTransactionDBTest, TransactionDBTest (both the TransactionDB- level and Transaction-level overloads); three new RocksIteratorTest cases, including one that asserts a key(Mapper) view used after next() throws IllegalStateException — the regression the arena-scoping specifically exists to prevent for the iterator path. Full suite: 443 tests, 0 failures. javadoc:javadoc: zero output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Update: expanded scopeThis PR grew well past the original ReadWriteDB-only `withPinned`. Summary of what changed since the title/description above:
Test coverage expanded to match: Full suite: 443 tests, 0 failures, 0 errors. |
| } | ||
| long len = lenSegment.get(ValueLayout.JAVA_LONG, 0); | ||
| MemorySegment view = data.reinterpret(len, arena, null).asReadOnly(); | ||
| return fn.map(view); |
There was a problem hiding this comment.
we should have same guarantee of get() => the mapper should not return null here
(same for value() I guess)
There was a problem hiding this comment.
Fixed — RocksIterator.key(Mapper)/value(Mapper) now reject a null return from fn with NullPointerException (Objects.requireNonNull), same as get(key, Mapper). Also tightened Mapper#map's own Javadoc to state the non-null contract once, centrally. Added regression tests in RocksIteratorTest. Pushed in c7fe05d.
| } catch (Throwable t) { | ||
| throw RocksDBException.wrap("get_pinned failed", t); | ||
| } | ||
| return RocksDB.withPinnedCore(arena, err, pin, MH_PINNABLESLICE_VALUE, MH_PINNABLESLICE_DESTROY, fn); |
There was a problem hiding this comment.
this is duplicated several times... perhaps we should create a ticket to track it down (e.g. kill duplicated code by introducing PinnableSlice object)
There was a problem hiding this comment.
Agreed, filed #58 to track it — extract a PinnableSlice wrapper owning the single rocksdb_pinnableslice_value/_destroy mapping so RocksDB, Transaction, and TransactionDB stop each keeping their own copy. Leaving it out of this PR's scope since it touches call sites well beyond withPinned.
There was a problem hiding this comment.
looks like I'm crazy but second comment is claude actually :)
574757f to
6dfbd99
Compare
…terator Adds ReadWriteDB.get(MemorySegment, Mapper) and friends — a scoped, zero-copy read built on the rocksdb_pinnable_handle_t C API (rocksdb_get_pinned_v2/_cf_v2, rocksdb_pinnable_handle_get_value, rocksdb_pinnable_handle_destroy) as an alternative to get(byte[]) for hot read paths, extended to every DB wrapper in this codebase: - ReadWriteDB, ReadOnlyDB, TtlDB, SecondaryDB, BlobDB, OptimisticTransactionDB — backed by a plain rocksdb_t*, share the rocksdb_pinnable_handle_t mechanism via RocksDB.withPinned/withPinnedCf. - TransactionDB and Transaction — no _v2 equivalent in c.h, so these go through the older rocksdb_pinnableslice_t API instead, sharing the same lifetime contract via RocksDB.withPinnedCore (parameterized on the value/destroy MethodHandle pair for whichever native handle kind applies). - RocksIterator.key(Mapper)/value(Mapper) — same zero-copy-view-scoped-to- the-callback idea, turning "escaped view silently reports the next position's bytes" into a loud IllegalStateException/WrongThreadException instead. Mapper<R> is the callback: R map(MemorySegment value), invoked with a read-only view bound to an arena that closes the moment the callback returns. A null return from the mapper is rejected with NullPointerException everywhere it's used, rather than silently conflated with "key not found" (Optional.empty()) or accepted as a valid result. Also: - NativeLibrary.lookup grows a Linker.Option... parameter so rocksdb_pinnable_handle_get_value/destroy (pure pointer arithmetic, no heap-array access) can be marked Linker.Option.critical(false). get_pinned_*_v2 stays non-critical since it can block on disk I/O. - JMH: new Instant-deserialize comparison (byte[] get + manual deserialize vs. zero-copy Mapper) and a blob-size sweep (8B..1MB) across byte[] get, get(MemorySegment,MemorySegment), and get(key, Mapper) to find the crossover point. Test coverage: PinnedGetTest plus per-DB-type happy-path/absent-key/CF/ thread-escape/null-mapper coverage across BlobDBTest, OptimisticTransactionDBTest, ReadOnlyDBTest, SecondaryDBTest, TransactionDBTest (both TransactionDB- and Transaction-level), TtlDBTest, and RocksIteratorTest. Follow-up tracked in #58: rocksdb_pinnableslice_value/_destroy are still mapped independently in RocksDB, Transaction, and TransactionDB rather than through a single PinnableSlice wrapper — pre-existing duplication, not introduced here, left out of scope.
Summary
Implements #55. Adds
ReadWriteDB.withPinned(...)— a scoped, zero-copy read built on therocksdb_pinnable_handle_tC API (rocksdb_get_pinned_v2/_cf_v2,rocksdb_pinnable_handle_get_value,rocksdb_pinnable_handle_destroy), as an alternative toget(byte[])for hot read paths.PinnedReader<R>— functional callback,R read(MemorySegment value) throws Exception, invoked with a read-only view of the pinned value. No intermediatebyte[]copy.RocksDB.withPinned0— private core shared by both public overloads (plain and column-family). Checkserrptrbefore the null-handle check (aNULLreturn means either NotFound or error — the two are only distinguished byerrptr), binds the value pointer to a confinedArenaviareinterpret(len, arena, null)(thenullcleanup is deliberate — the view borrows from the handle, it does not own the memory).finallyblock closes the arena before destroying the native handle. Reversed, there'd be a window where a still-validMemorySegmentpoints at block-cache memory the handle already released. With this order, a view that escapes the callback fails loudly:IllegalStateExceptionif used after the call returns,WrongThreadExceptionif handed to another thread — both covered by tests, both firing as expected (not silently passing).get_pinned_*_v2is not marked critical (aGetcan block on disk I/O; a critical downcall would stall GC for its duration).get_value/destroyareLinker.Option.critical(false), which neededNativeLibrary.lookupto grow aLinker.Option...parameter.Deviation from the issue's sketch: no separate
RocksPinnedclass with its ownLinker/SymbolLookup. The four method handles live inRocksDB.javanext to the existingget_pinned/PinnableSlicehandles — that's where every otherrocksdb_t*handle in this codebase lives (RocksDB.java's own doc comment calls it out as "the single holder of allrocksdb_t*method handles"). The CF parameter is the existingColumnFamilyHandlewrapper rather than a rawMemorySegment, matching every other CF-taking method here.Test plan
WithPinnedTest(11 cases): happy path, absent key, empty value, escape-after-return (IllegalStateException), escape-to-another-thread (WrongThreadException), reader exception propagates and cleanup still runs, 50k-iteration loop as a leak smoke test (scaled down from the issue's 100k-with-RSS-measurement to fit a fast unit-test suite), CF overload (happy/absent/isolation), and a default-CF equivalence test provingwithPinnedandwithPinned(cf, ...)are wired to the right native symbols../mvnw test -pl core -am— 415 tests, 0 failures, 0 errors./mvnw javadoc:javadoc -pl core— zero outputFfmBenchmark: anInstantstored as an 8-byte long, read viaget(byte[])+ manual deserialize vs.withPinneddeserializing straight out of the pinned view. Quick local run (1 fork, short warmup): ~7.0M ops/s (byte[]) vs. ~8.1M ops/s (withPinned), consistent with skipping one heap allocation + copy per read.BenchmarkRunnerlists both as FFM-only (no JNI equivalent), same treatment as the existingMemorySegmenttier.🤖 Generated with Claude Code