LAB-347: perf: release the GIL during ByteStorage compress/hash - #223
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Expert-panel crypto/protocol gate — ByteStorage GIL release (automated review-signoff sweep, deep-executor)Ran the mandatory expert panel (high-stakes; the ByteStorage store/retrieve path sits beneath the AES-GCM encryption layer, so Ray's crypto/protocol gate applies to any change here). HEAD Verdict: SHIP. The GIL-release change does NOT alter the wire format, encryption, AAD, key derivation, or cache-key format — it is a pure concurrency change with byte-identical output (verified by
Finding dispositions (both non-blocking, test-diagnostics only)
Neither MIN touches production code; the 14-line binding change is unanimously clean. Not re-opening CI convergence for two test-comment/diagnostics nits. CI green. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 24 minutes. |
ByteStorage.store()/retrieve() ran the entire LZ4 + xxh3 + msgpack core under the GIL, freezing every other Python thread for the full duration of a large payload (up to 512MB). Wrap the pure-Rust core of store, retrieve, estimate_compression and validate in Python::detach (pyo3 0.29 name for allow_threads) so concurrent Python threads keep running. The companion full-payload data.to_vec() copy was already eliminated in cachekit-core 0.3.0 (cachekit-io/cachekit-core#48), which this repo already pins; a subprocess peak-RSS invariant now guards the whole FFI stack against that copy regressing (512MB store peaks at 1.24x payload; the copy would push it to ~2.2x). No change to the on-disk/on-wire envelope format: the diff touches only GIL handling in the bindings, and round-trip + backward-compat tests pass unchanged. The GIL tests were verified to FAIL against the previous bindings (interior-window ticker proof: with the GIL held, a concurrent thread makes zero progress inside the middle 50% of the call window). Closes cachekit-io/cachekit-core#45 Co-authored-by: Winston <winston@27b.io> Co-authored-by: multica-agent <github@multica.ai>
c1a9840 to
980de83
Compare
Problem
ByteStorage.store()/retrieve()ran the entire LZ4 compress + xxh3 hash + msgpack serialize core under the GIL. A large payload (up to 512MB) froze every other Python thread for the full duration — ~1.8s for a 256MB incompressible payload. (cachekit-io/cachekit-core#45, LAB-347)Fix
Wrap the pure-Rust core of
store,retrieve,estimate_compressionandvalidateinPython::detach(pyo3 0.29's name forallow_threads). Sound because&[u8]args borrow immutablebytesbuffers kept alive by the call frame.estimate_compression/validatedo the same full-payload compression/decompression work, so they get the identical one-line treatment.The companion full-payload
data.to_vec()copy named in core#45 was already eliminated in cachekit-core 0.3.0 (cachekit-io/cachekit-core#48), which this repo already pins — verified:StorageEnvelope::newtakes&[u8], and Cargo.lock resolves cachekit-core 0.3.0 from crates.io.Acceptance criteria → evidence
tests/critical/test_byte_storage_gil.py— a ticker thread must timestamp inside the middle 50% of a large store/retrieve call window (margins dwarf the ~5ms GIL switch interval, so this is deterministic in both directions). Verified to FAIL against the previous bindings (rebuilt withoutdetach: both tests fail; with it: pass).tests/performance/test_large_object_memory.pyguards the whole FFI stack: 512MB compressible store peaks at 1.24× payload (the old copy would push ~2.2×; bound set at 1.7×). Runs in the existing CI memory-invariant step. Remaining copies are the unavoidable ones: Rust buffer →PyByteson return.rust/src/python_bindings.rs— zero changes to envelope layout, AAD, key derivation, or wire format, so the crypto/protocol review gate is not triggered. Encryption bindings deliberately untouched.Also verified locally
cargo fmt --check,cargo clippy --locked -D warnings,ruff format/check .,basedpyright --level error, rust unit tests, and all three CI pytest selections (unit-m "not slow" -n auto, critical-m "not slow", performance-m "performance and slow").Follow-up observed (out of scope)
The msgpack envelope encodes
compressed_data: Vec<u8>as an array of integers (noserde_bytes), inflating incompressible payload envelopes to ~1.58× and dominating store cost (~150MB/s). Fixing it changes the wire format → protocol-gated, separate issue.Closes cachekit-io/cachekit-core#45