fix(cache): count and bound bytes held outside the memory index - #44
fix(cache): count and bound bytes held outside the memory index#44zfarrell wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| let victims = 8; | ||
| let cache = LiquidCacheBuilder::new() | ||
| .with_max_memory_bytes(entry_bytes * victims) |
There was a problem hiding this comment.
nit: this test never exercises a group holding more than one victim (not blocking).
squeeze_in_flight_limit() returns entry_bytes * 8 / 32, which is entry_bytes / 4. insert_inner reserves entry_bytes for the pending entry at core.rs:359 before calling squeeze_victims. group_victims_by_in_flight_bytes then saturates limit to 0 at core.rs:541. Grouping therefore produces eight groups of one victim, and the concurrent path runs sequentially.
The other new test builds multi-victim groups but asserts nothing about the peak. So no test covers the bound when a group holds several victims.
Raise max_memory_bytes here so limit admits several victims. Then assert the peak stays under squeeze_in_flight_limit() plus the pending entry.
| /// outputs in memory at once, which is unbounded in bytes however few | ||
| /// victims there are: eight 60 MB batches is half a gigabyte of transcode | ||
| /// output outside the index. A squeeze's output cannot exceed its input, so | ||
| /// each victim's indexed size bounds what it will add, and grouping by that |
There was a problem hiding this comment.
nit: the stated bound does not hold on every squeeze path (not blocking).
TranscodeSqueezeEvict returns a memory_squeezed_liquid entry and a full bytes_to_write buffer at policies/squeeze.rs:111-114 and again at policies/squeeze.rs:146-149. squeeze_victim_inner resizes the reservation to the sum of both at core.rs:611-614. That sum can exceed the entry's indexed size, so a group can hold more than squeeze_in_flight_limit().
Reword the comment to call the indexed size an approximate bound rather than a hard one.
There was a problem hiding this comment.
Reviewed the full diff. Accounting is balanced: every reservation has a matching release on both the success and the error paths, and reset_usage correctly leaves in_flight_memory_bytes alone so outstanding reservations cannot underflow it. Two nits left inline. CI checks were still queued at review time, so test results are unverified.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
1 participant
Addresses part of #43: the memory tier's budget counted an entry only once it had landed in the index, so every intermediate the hydrate → insert → squeeze cycle held on the way there was invisible to it.
BudgetAccountingnow tracks in-flight bytes — disk decodes, squeeze output awaiting its write and insert, and entries pending admission — via an RAII reservation, exposed asin_flight_memory_bytesand a peak alongsidememory_usage_bytes(transients die between gauge scrapes, so the peak is what a scrape can catch). Reservations report and bound; they never gate admission, per the issue's staging.squeeze_victimsno longer fans out withbuffer_unordered(usize::MAX): victims are grouped so their summed indexed sizes stay undermax_memory_bytes / 32, which bounds bytes rather than count — a squeeze's output cannot exceed its input, so the indexed sizes are a sound bound.Measured on 8 liquid victims of 6,241 B: peak in-flight is 18,723 B bounded vs 55,770 B unbounded (≈ 9× one entry, i.e. all eight outputs plus the pending entry resident together — the issue's claim, now under test). Only victims whose squeeze writes to disk pile up; an arrow victim's transcode never awaits, so its future completes in one poll and never overlaps a sibling. The concurrent squeeze path — the one that ships, since
builders.rsonly disables it undercfg(test)— now has coverage: entries read back byte-identical, the tally matches the sum of indexed entry sizes, and in-flight drains to zero.Directions 2 (conditional hydration) and the gating half of 1 are deliberately left out; they need the gauge's numbers from the fixture first. I have no access to that fixture or cluster, so the issue's acceptance criteria — the
get_arrow_array_with_filter → squeezeprofile edge, anon peak, and the 240k warm p50 — are unverified here and need a run by someone who does.What this is not expected to do: move the anon peak on the fixture. The observed OOM is dominated by the arrow→liquid transcode, and that arm (
squeeze.rs:116-119) returnsbytes_to_write: None— no write, so no await, so those futures were already polled to completion one at a time and never overlapped. Grouping has nothing to bound there. What should change is the visibility:peak_in_flight_memory_bytesshould come back distinctly non-zero, becausesqueeze_victim_innerreserves the victim's indexed size across exactly the window the profile attributes 43-49% of live heap to. If it instead reads near zero on a run that OOMs, the reservation is missing the transient that actually dominates and the accounting needs rework before direction 2 is designed on top of it.