test(perf): extend peak-RSS suite to backend-inclusive File read paths (LAB-350) - #243
Merged
Merged
Conversation
…s (LAB-350) The memory suite measured only the serializer layer with the read input pre-allocated, so backend read-side copies (File os.read + header slice) were structurally invisible — a regression reintroducing a full-payload read-side copy passed the suite (cachekit-py#169). Add three backend-inclusive guards running the read END-TO-END through FileBackend -> StandardCacheHandler -> CacheOperationHandler: - mmap fast path (tracemalloc): ~1.1x logical on the Python heap; the os.read fallback regression measures ~1.9x (fault-injected) and trips the 1.6x bound. - non-mmap default-serializer path (tracemalloc): pins the measured ~5x cost (os.read + slice + bytes() coercion of the zero-copy view + envelope decode + output) so one MORE full-payload copy fails; reducing the 5x is follow-up work per #169's scope. - end-to-end peak RSS (subprocess): ~2x payload (mapped pages + df); catches full-payload copies tracemalloc cannot see (Arrow pool, Rust, view coercion — fault-injected at 3.0x vs the 2.6x bound). Subprocess peak-RSS now reads VmHWM from /proc/self/status instead of ru_maxrss: ru_maxrss survives fork+exec on Linux, so a child spawned from a fat pytest process inherits the parent's watermark and real regressions hide under it (observed: 2GB inherited base, 0.00x measured cost). Applied to the existing ByteStorage store guard too, which had the same silent false-pass fragility.
27Bslash6
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…B-350) Panel finding (fault-verified): to_pandas(self_destruct, split_blocks) returns numpy VIEWS over Arrow-pool buffers, so the healthy mmap-path read allocates ~0.001x logical on the Python heap — not the ~1.1x the docstring claimed. Under the old 1.6x bound a reintroduced bytes() coercion of the zero-copy view (measures ~0.96x) silently PASSED. Tighten the bound to 0.5x: both regression classes now trip it (coercion ~1.0x, os.read fallback ~1.9x), and a future pyarrow that copies in to_pandas fails loud for conscious recalibration. Also from the panel: module docstring no longer claims the suite avoids process RSS (three tests are RSS-based since this PR); the two-scopes explanation lives once (section banner) with the module docstring pointing at it; README catalog entry no longer duplicates multipliers that live in docstrings; RSS-test baseline folded into the assert message instead of a capture-swallowed print.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughPerformance documentation and memory regression tests now use Linux VmHWM measurements and cover Python allocation and peak-RSS bounds across serializer-only and File backend-inclusive read paths. ChangesMemory Regression Guards
Estimated code review effort: 4 (Complex) | ~40 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Contributor
Author
|
@kody start-review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Author
|
@kody start-review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/performance/test_large_object_memory.py`:
- Around line 351-356: Add explicit timeouts to both subprocess.run calls in the
write/read execution flow, using an appropriate bounded duration so wedged child
processes fail the test instead of hanging CI. Preserve the existing
trusted-command S603 suppressions and return-code assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbfddb52-d538-4393-88f3-c15715da6196
📒 Files selected for processing (2)
tests/performance/README.mdtests/performance/test_large_object_memory.py
CodeRabbit: subprocess.run without timeout lets a wedged child hang the CI job to its ceiling. All three measurement subprocesses (ByteStorage store guard + the RSS write/read pair) now share a 300s timeout — generous for slow runners, and a hang fails the test loudly via TimeoutExpired instead of stalling the run. Co-authored-by: multica-agent <github@multica.ai>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
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
1 participant
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.
Resolves LAB-350 · Addresses the File-backend portion of #169
Problem
The peak-RSS / OOM-fix suite (
tests/performance/test_large_object_memory.py) measured only the serializer layer, with the read input allocated before measurement started. Backend read-side copies —FileBackend.get()'sos.read+file_data[14:]slice — were structurally invisible, so a regression reintroducing a full-payload read-side copy passed the suite while the headline low-read-memory claim quietly broke.What this adds
Three backend-inclusive guards that run the read end-to-end through the real stack (
FileBackend → StandardCacheHandler → CacheOperationHandler → unwrap → deserialize), clearly separated from the serializer-only tests so the headline claim maps to the right number:test_file_backend_read_python_allocations_bounded(mmap fast path, #171)os.readfallback)test_file_backend_bytes_read_python_allocations_bounded(default serializer)test_file_backend_end_to_end_read_peak_rss_boundedEvery bound was verified by fault injection in both directions: the healthy path passes with margin, and a simulated full-payload read-side copy fails. The two metrics are deliberately complementary — the mmap→
os.readfallback swaps file-backed pages for anonymous heap at the same ~2x RSS total (tracemalloc catches it at 1.9x), while Arrow-pool/Rust/view-coercion copies are invisible to tracemalloc (RSS catches them at 3.0x).Surfaced (not fixed here — out of scope per the ticket)
The non-mmap default-serializer read costs ~5x payload on the Python heap:
os.read+ header slice (the #169 copies),bytes(data)re-coercion of the envelope's zero-copy memoryview inStandardSerializer.deserialize(Rust retrieve needs bytes), the decompressed msgpack document, and the unpacked output. The new test pins this at its current level; reducing it is a follow-up ticket.Measurement-integrity fix
Subprocess peak-RSS now reads
VmHWMfrom/proc/self/statusinstead ofresource.ru_maxrss:ru_maxrsssurvives fork+exec on Linux (it lives in the signal struct, not the mm replaced by exec), so a child spawned from a fat pytest process inherits the parent's watermark and real regressions hide under it — observed as a 1997 MiB inherited base with a 0.00x measured read cost. The pre-existing ByteStorage store guard had the same silent false-pass fragility and got the same fix (now also explicitly Linux-only; on macOSru_maxrssis bytes, not KiB, so it was already de-facto Linux-only).CI budget
All new tests are
performance and slow(the memory-invariant CI job). Full selection: 11 passed in ~11s locally (~7s added).Docs
tests/performance/README.mdnow catalogs the memory suite and the two measurement scopes; the module docstring explains the serializer-only vs backend-inclusive split. No public docs (docs.cachekit.io, protocol) state measurable read-RSS numbers, so nothing else needed changing.Summary by CodeRabbit
Documentation
Tests