Repository navigation
perf: optimize decoded MultiGet reads - #24
Conversation
Compare unchanged main, PR #17 and constrained decoding. Include key packing, A/A controls and two launches on AMD64 and ARM64.
Keep the database lease through decoding without acquiring it twice. Restore scalar error scanning: paired AMD64 results did not show a consistent benefit from the span scan.
Keep one decoded implementation and forward the interface overload. Move the comparative harness to perf/multiget-bench-proof. Cover full payloads and document sorting and native value lifetimes.
benaadams
left a comment
There was a problem hiding this comment.
Re-review at 6f729e0. The earlier findings are resolved:
- The native batch path is now the stated design.
- The harness and the experiment workflows are gone.
- All six native libraries were rebuilt: each sha256 matches its LFS oid against the artifacts of run 38880619766, built at d97799e. No native source or build workflow changed between that build and the head.
- Tests are green on all six platforms in run 38885742490.
There is one Medium, inline: the cache-only read tier is silently ignored for named families or an explicit default handle. The rest is Low or Nit.
Not inline:
-
Test gaps:
- No present-but-empty value goes through the 2-key path.
_WithMultiByteValueswrites[]only whencount > 2, and_WithOnlyMissingOrEmptyValuesstarts at 3. CountingValueDeserializeronly runs underSetReadTier(1), so no stateful struct decoder reaches either batched loop._WhenDecoderClosesDatabasedoesn't cover 2 keys or a named family.
Each of these lets a regression pass, for example skipping zero-length values on the 2-key path, or copying the struct decoder by value inside a loop.
- No present-but-empty value goes through the 2-key path.
-
checked(count * 2)throwsOverflowExceptionabove 2^30 values after the output has been cleared. That exception, and theInvalidOperationExceptionfrom the native path, are undocumented. The interface overload also cannot passsortedInput. -
AGENTS.md:9says the native declarations are generated, but the batch header is bound by hand. Thegit-lfsinstall atbuild-rocksdb.yml:50looks unused, and the reason for thesafe.directorystep (:73-78) is not stated. -
The PR body still says the comparison table is "pending" and "CI is being repeated". It cites run 38884441416, which is at 391ccbd, not the head. It also has no measurements for a
perf:change.
Honor non-default read tiers for every column family. Reject disposed handles before native reads, including decoder reentry. Contain native exceptions and remove profiling-only code.
benaadams
left a comment
There was a problem hiding this comment.
All review points are addressed at cb3bcdc:
- The read tier is honoured for every family on both batched paths.
- Disposed family handles are rejected, both at entry and before each scalar read.
- The jemalloc probe is gone.
- Native exceptions are contained.
- The generic cases run a struct decoder.
- The exception docs and the
sortedInputoverload are in.
One thing to land before merge: native/rocksdb_batch.inc and rocksdb_batch.h changed in this commit, but the six packaged libraries are still the ones built at d97799e (run 38880619766), so they still export rocksdb_net_thread_allocated and lack the catch (...). Please commit the binaries from run 38890192099 and re-run the packaged tests on them.
|
The rebuild is already in progress in run 38890192099. Five platforms have passed; macOS x64 is still building. I'll commit all six binaries and rerun the packaged tests when it finishes. |
|
All six binaries from 38890192099 are now committed at 1b400f9. Their hashes match the build artifacts, and the packaged tests passed on all six platforms in 38892870090. The native threads are resolved. The final benchmark is running on this revision in 38892956847. |
Add decoded fixed-length
MultiGetoverloads that write into a caller-owned span. The generic overload supports struct decoders without boxing. The interface overload forwards to it and supports the same sorting hint. This includes the API proposed in #17; its separate implementation is not needed.One key uses the existing pinned-read helper. Two keys use the existing batched C API. Larger batches share one native owner and avoid per-value wrapper allocations and native getter/destroy calls. All-missing batches skip owner allocation. Missing and empty values remain distinct. Database and read-options leases stay held until native cleanup completes.
Non-default read tiers use scalar reads for every family. A scalar read failure retains the decoded prefix and leaves the tail cleared. Native batch errors are checked before decoding. Disposed family handles are rejected at entry and before each scalar read. Native exceptions cannot escape the new batch C API.
The PR contains the managed API, native extension, six rebuilt native libraries, regression tests and API documentation. Build dependencies and CPU requirements are unchanged. The comparison harness, frozen references and proof scripts are outside this PR.
Validation
cb3bcdc. Every packaged LFS object at1b400f9matches its verified build artifact. The removed profiler export is absent from all six libraries.1b400f9. The expanded suite has 549 tests on Linux. The solution compiles with zero warnings and errors.1b400f9, verified native sources, build results and binary hashes, then ran packaged tests and measurements. Each architecture has 117 complete BenchmarkDotNet results with two launches, plus three paired processes.Measurements
Time change of the new generic overload with a struct decoder. Negative means less time. Each cell is the median of three process medians. Each process uses twelve balanced ABBA/BAAB rounds on one pinned CPU, with matched A/A controls.
*means at least one process did not separate from zero and its control variation; it is not a confirmed gain. The bands describe observed variation, not confidence intervals.AMD64: AMD EPYC 7763. ARM64: Neoverse-N2. Baselines: main
165c9d2, #17d6040cf, previous #24f189a51. All methods use the same rebuilt native library. Main is the existing array-returning API; the candidate uses prepacked contiguous keys and a caller-owned output span. BenchmarkDotNet also measures key-packing costs separately.Unless stated otherwise, cases use 32-byte keys, 128-byte values, 100% hits, a named column family and warm SST data.
The struct decoder took less time than main in all thirteen cases, beyond the matched controls in all three processes on both architectures.
The two-key class decoder has a small measured time increase versus the previous #24: the median of process medians is +0.91% on AMD64 and +0.14% on ARM64. Two AMD64 processes and one ARM64 process exceeded their control bands; the other class two-key processes were inconclusive. The struct two-key changes versus the previous #24 were also inconclusive. These results do not support a blanket claim of no performance regressions versus the previous implementation.
The 7,488 candidate samples in the paired comparisons recorded zero managed allocations and zero GC collections. BenchmarkDotNet also reported zero managed bytes for all Candidate, CandidateClass and CandidatePacked cases on both architectures. In the 32-key paired case, main allocated 228.625 managed bytes per key; the new overload allocated zero. These allocation results apply to the measured numeric decoders and warmed paths.
Raw data: AMD64, ARM64.
Callers must use the new overloads to get these gains. The existing allocating overload is unchanged. These are warm component benchmarks; they do not establish RPC or block-processing gains. The native owner retains value pins until decoding completes. Slow decoders and cache pressure are outside this matrix.