Repository navigation
Conversation
jsam
force-pushed
the
fm/il-384-recursive-reads
branch
from
October 6, 2026 05:18
b3a98a1 to
669d480
Compare
…, not after every one (#384) Recursive reads were 9-20% slower since #304. The bisect blamed the per-step deadline and memory checks inside recursion, but on the benchmark host at main they cost nothing (?hot_reach(X,Y) over 100K edges: 275.7 ms with them, 275.1 ms with them stubbed). The cost is the release of freed memory #304 added: after every computation that peaked over 64 MiB, the server purged the allocator's freed pages on the reply path, and the next read faulted the same pages back in. With that release stubbed the same read takes 223.5 ms (-19%), ?path(400,Y) 220.7 ms instead of 265.4 ms. The release now waits until no large computation has finished for one second, then runs once on the blocking pool: a stream of large reads keeps reusing its pages, and the memory goes back to the operating system within a second of the last one. The stop guards are untouched, and the memory a stream holds meanwhile is bounded as before by the limits its computations ran under; resident memory can stay at the stream's working size for up to a second longer than before. The perf gate now has the case: `recursive_query`, `?hot_reach(X, Y)` of the views benchmark's rules over the delta_keyed graph (100K edges), 25 serial reads per round, judged on p50.
jsam
force-pushed
the
fm/il-384-recursive-reads
branch
from
October 6, 2026 06:36
669d480 to
8be9c5d
Compare
Contributor
Author
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
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.
Intent
Fix GitHub issue #384 in inputlayer/inputlayer (production readiness, performance): "Recursive reads 9-20% slower since #304 (runaway-query guards): ?hot_reach(X,Y) 248 -> 301 ms at 100k edges".
Found by the #378 bisect on the benchmark host. Likely the per-step deadline/memory checks inside recursion. Make the check cheaper (check every N steps) without weakening the guard; add the case to the perf gate.
Deliver it as a small PR; done means merged with CI green.
What Changed
src/protocol/handler/supervise.rs: a computation whose memory peak exceedsRELEASE_AFTER_PEAK_BYTESno longer returns freed memory to the OS inline before dropping its permit. A newMemoryReleaserruns the release on the blocking pool once no large computation has finished for 1 s (RELEASE_QUIET), and at the latest 5 s after the first one pending (RELEASE_LATEST), so back-to-back large reads share one release instead of paying a purge and page re-fault each (about 50 ms per read, per the Recursive reads 9-20% slower since #304 (runaway-query guards): ?hot_reach(X,Y) 248 -> 301 ms at 100k edges #384 analysis recorded in the code comment). Query deadline and memory-limit checks are untouched; two paused-clock tests cover the quiet-period release and the five-second cap.perf-gate: new gatedrecursive_queryfixture — 25 serial?hot_reach(X, Y)reads (3 warmup) over the deployedpath/hot_reachrules on 100K edges — withrecursive_query.latency_us.p50added to therequiredset inpolicy.toml. The hot-chain graph loader (load_hot_chains,hot_head) moves out ofkeyed.rsintofixtures/mod.rsso both fixtures share it; the README documents the new fixture.gui/lib/docs-bundle.ts) now describes the deferred release, noting that freed memory can stay resident for those few seconds while the query and server memory limits apply as before.🤖 Generated with Claude Code
Risk Assessment
✅ Low: The deferred, capped release is a small self-contained state machine whose traces (quiet release at last+1 s, cap at first+5 s, new window after each release) hold, the accounted memory limits are untouched, and the new gated fixture's expected row count and sample count agree with the dataset and policy; the remaining trade-offs (remedy differing from the intent's wording, freed pages resident for up to 5 s) were already decided by the user.
Testing
I built release servers for the base commit and this change and ran the new
recursive_querygate fixture against both: p50 was 419.9 ms at base and 353.3 ms with the change (cost ratio 0.795, interval 0.723..0.885), and the gated metric passed. The host was busy (1-minute load 58 on 32 CPUs, baseline spread 52%), so the absolute numbers are noisy; the direction is clear from the paired rounds. The gate's overall verdict reads INVALID only because I ran one fixture of eleven; the full gate was not run here. With a small WebSocket driver against the real servers I then checked memory and guards: after a stream of reads the change returns memory about 1.0 s after the last read (base: after every read); under a stream that never goes quiet, about 1 GiB left by a big query went back 5.0 s after it finished; a query was refused withresource_exhaustedat a 16 MiB limit and at the default 4 GiB limit, and a runaway recursion withdeadline_exceededat 2 s, with the server answering normally afterwards. The module's unit tests, including the two paused-time releaser tests, pass, but that is a unit-test result and not a live one, so the controlled-time releaser scenario is recorded as untested. No UI is involved, so there are no screenshots. Build outputs from this run were removed from the worktree and no server was left running.?hot_reach(X, Y)over 100K edges repeatedly; the read is faster than at the base commit and returns the right 1,002 rowsscripts/perf-gate.sh --baseline-rev d5df1ba3 --fixtures recursive_query --rounds 6: p50 419.9 ms base vs 353.3 ms change, cost ratio 0.795 (interval 0.723..0.885); recursive-report.mdrecursive_querycase as a required metricrecursive_query.latency_us.p50under Required metrics with status PASS and row counts checked on every timed reply; recursive-report.mdresource_exhaustedand the server keeps serving?hot_reach(X, Y)refused after 69 ms at a 16 MiB limit,?edge(0, Y)answers afterwards; rss-cap-big.log: a cross join refused at the default 4 GiB limit with RSS peaking at 4.2 GiB?count(X)overcount(Y) <- count(X), Y = X + 1refused withdeadline_exceededafter 2002 ms atquery_timeout_ms = 2000; the next query answerscargo test --lib protocol::handler::supervise, a unit test under paused Tokio time, which is not the run…Evidence: Gate result for recursive_query, base vs change
Evidence: Gate report (full)
Evidence: Gate run log (builds and measurement)
Evidence: Memory returned after a second of quiet (change vs base)
[candidate] idle RSS before the stream: 44 MiB [candidate] RSS while the stream runs: peak 161 MiB, lowest 106 MiB [candidate] RSS 1000 ms after the last read: 138 MiB [candidate] RSS 1250 ms after the last read: 44 MiB [candidate] RSS fell halfway to its final level 1012 ms after the last read [base] RSS while the stream runs: peak 144 MiB, lowest 53 MiB [base] RSS fell halfway to its final level 15 ms after the last readEvidence: Five-second cap under a stream that never goes quiet (change vs base)
[candidate] ?edge(X, Y), label(A, B) -> resource_exhausted after 5662 ms; RSS peaked at 4200 MiB [candidate] then 34 ?hot_reach(X, Y) reads back to back; longest gap between two large computations finishing: 363 ms [candidate] 4.5-5.0 s after the big query: RSS 1120..1157 MiB [candidate] 5.0-5.5 s after the big query: RSS 146..1155 MiB [candidate] 5.5-6.0 s after the big query: RSS 199..233 MiB [base] 0.0-0.5 s after the big query: RSS 114..209 MiBEvidence: Sudden RSS drops during 14 s of back-to-back reads (change 5, base 39)
Evidence: Guards still stop queries on the change
max_query_memory_bytes = 16777216 ?hot_reach(X, Y) -> {"type":"error","message":"Request exceeded the per-query memory limit (storage.performance.max_query_memory_bytes) before it began committing; nothing was applied. ...","code":"resource_exhausted"} after 69 ms ?edge(0, Y) afterwards -> {"rows":1} query_timeout_ms = 2000 ?count(X) (recursion with no fixpoint) -> {"type":"error","message":"Request deadline exceeded before it began committing; nothing was applied","code":"deadline_exceeded"} after 2002 ms ?edge(0, Y) afterwards -> {"rows":1}~/.no-mistakes/evidence/01M47C9D1HYNPRHQFEKNS2N22C/drive.mjs)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
🔧 **Rebase** - 1 issue found → auto-fixed ✅
gui/lib/docs-bundle.ts- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Review** - 3 issues found → auto-fixed ✅
src/protocol/handler/supervise.rs:98- Intent conformance: the intent says "Make the check cheaper (check every N steps) without weakening the guard". The change leaves the per-step deadline/memory checks untouched and instead changes a different part of fix: refuse or stop runaway cross-product queries within their limits and release their memory #304: the post-query release of freed memory (if job_control.memory_peak() > memory::RELEASE_AFTER_PEAK_BYTES { RELEASER.finished_large(RELEASE_QUIET, memory::release_freed_memory); }replaces the immediatememory::release_freed_memory()before the permit drops). The commit message justifies this with host measurements (checks stubbed: 275.7 vs 275.1 ms; release stubbed: 223.5 ms), which I cannot verify from source. The intent's diagnosis was hedged ("Likely"), but the prescribed remedy was not implemented and a user-visible memory behaviour (documented in deployment.mdx) was changed instead. Please confirm that deferring the release is the accepted fix for Recursive reads 9-20% slower since #304 (runaway-query guards): ?hot_reach(X,Y) 248 -> 301 ms at 100k edges #384 and that it does not count as weakening the guard.src/protocol/handler/supervise.rs:143- The release is a pure debounce with no upper bound: every large computation pusheslast_largeforward and the task loops onsleep_until(last_large + quiet). Under a sustained stream where a >64 MiB computation finishes at least once per second (exactly the workload this optimises, e.g. clients looping ~250 ms recursive reads),releasenever runs. Concrete sequence: one query peaks near its memory limit, then a steady stream of ~70 MiB reads follows on other blocking-pool threads (other jemalloc arenas); the big query's freed pages are not reused by the stream and are no longer purged by the server, only by jemalloc's own ~10 s decay, which is the situation fix: refuse or stop runaway cross-product queries within their limits and release their memory #304 added the explicit release for. Before this change they were purged before the permit admitted the next computation. The doc comment and commit claim "resident memory meanwhile is bounded as before, by the memory limits" and "goes back within a second of the last one", but the limits count live bytes, not dirty pages in idle arenas, so RSS can sit above them for longer than before. The limits themselves (accounted bytes, not RSS) are not weakened. Smallest remedy would be a cap on total deferral (release at the latest N seconds after the first pending large computation); that changes the chosen trade-off, so it needs the author's decision.src/protocol/handler/supervise.rs:430-freed_memory_is_released_once_large_computations_go_quietrelies on wall-clock sleeps with a 70 ms margin: theRELEASED == 0assertion fails if the five 30 ms sleeps overshoot by a total of 70 ms after the lastfinished_large, and the== 1/== 2assertions fail if the spawned task plusspawn_blockingtake longer than ~80-100 ms to be scheduled on a loaded CI runner. The crate already enables tokiotest-utiland uses#[tokio::test(start_paused = true)]elsewhere (src/protocol/replication/follower.rs:648);MemoryReleaserusestokio::time::sleep_until, but compares againststd::time::Instant::now(), so making the test deterministic means usingtokio::time::Instantin the releaser as well. Thestarted.elapsed() >= 4*30ms + quietassertion at line 433 is always true given the sleeps before it and proves nothing.🔧 Fix applied.
4 issues (2 warnings, 2 infos) still open:
src/protocol/handler/supervise.rs:98- Intent conformance: the intent says "Make the check cheaper (check every N steps) without weakening the guard". The change leaves the per-step deadline/memory checks untouched and instead changes a different part of fix: refuse or stop runaway cross-product queries within their limits and release their memory #304: the post-query release of freed memory (if job_control.memory_peak() > memory::RELEASE_AFTER_PEAK_BYTES { RELEASER.finished_large(RELEASE_QUIET, memory::release_freed_memory); }replaces the immediatememory::release_freed_memory()before the permit drops). The commit message justifies this with host measurements (checks stubbed: 275.7 vs 275.1 ms; release stubbed: 223.5 ms), which I cannot verify from source. The intent's diagnosis was hedged ("Likely"), but the prescribed remedy was not implemented and a user-visible memory behaviour (documented in deployment.mdx) was changed instead. Please confirm that deferring the release is the accepted fix for Recursive reads 9-20% slower since #304 (runaway-query guards): ?hot_reach(X,Y) 248 -> 301 ms at 100k edges #384 and that it does not count as weakening the guard.src/protocol/handler/supervise.rs:143- The release is a pure debounce with no upper bound: every large computation pusheslast_largeforward and the task loops onsleep_until(last_large + quiet). Under a sustained stream where a >64 MiB computation finishes at least once per second (exactly the workload this optimises, e.g. clients looping ~250 ms recursive reads),releasenever runs. Concrete sequence: one query peaks near its memory limit, then a steady stream of ~70 MiB reads follows on other blocking-pool threads (other jemalloc arenas); the big query's freed pages are not reused by the stream and are no longer purged by the server, only by jemalloc's own ~10 s decay, which is the situation fix: refuse or stop runaway cross-product queries within their limits and release their memory #304 added the explicit release for. Before this change they were purged before the permit admitted the next computation. The doc comment and commit claim "resident memory meanwhile is bounded as before, by the memory limits" and "goes back within a second of the last one", but the limits count live bytes, not dirty pages in idle arenas, so RSS can sit above them for longer than before. The limits themselves (accounted bytes, not RSS) are not weakened. Smallest remedy would be a cap on total deferral (release at the latest N seconds after the first pending large computation); that changes the chosen trade-off, so it needs the author's decision.src/protocol/handler/supervise.rs:430-freed_memory_is_released_once_large_computations_go_quietrelies on wall-clock sleeps with a 70 ms margin: theRELEASED == 0assertion fails if the five 30 ms sleeps overshoot by a total of 70 ms after the lastfinished_large, and the== 1/== 2assertions fail if the spawned task plusspawn_blockingtake longer than ~80-100 ms to be scheduled on a loaded CI runner. The crate already enables tokiotest-utiland uses#[tokio::test(start_paused = true)]elsewhere (src/protocol/replication/follower.rs:648);MemoryReleaserusestokio::time::sleep_until, but compares againststd::time::Instant::now(), so making the test deterministic means usingtokio::time::Instantin the releaser as well. Thestarted.elapsed() >= 4*30ms + quietassertion at line 433 is always true given the sleeps before it and proves nothing.src/protocol/handler/supervise.rs:161- Latent, not reachable in the shipped server:RELEASERis a process-wide static, but the task that clearspendingis spawned on whichever Tokio runtime the large computation ran on. If that runtime is dropped while a release is pending (within 1-5 s of a >64 MiB computation), the task is dropped withpendingstillSome, and every laterfinished_largein the process only updateslastand returns, so the explicit release never runs again (jemalloc's own ~10 s decay still applies). The server binary builds exactly one runtime (src/bin/server.rs:65), so production is unaffected; the only processes that take this path are test binaries where each#[tokio::test]has its own runtime, and no test asserts on resident memory. Introduced by the original commit and kept by fix round 1 (which reshaped the state intoPending { first, last }). No action needed for this change; a drop guard in the spawned task that resetspendingwould close it if the handler is ever driven from more than one runtime.✅ No issues found.
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
?hot_reach(X, Y)over 100K edges repeatedly; the changed server answers faster than the base commitrecursive_querycase against real servers, checks the 1,002-row answer and judgesrecursive_query.latency_us.p50as a required metricdeadline_exceededafter 22 ms; the next full read returns 1,002 rowsresource_exhaustedand a small read still answersresource_exhausted;?edge(0, Y)returns 1 rowscripts/perf-gate.sh --baseline-rev 0056538513506df1f691369e28737e5ff6534436 --fixtures recursive_query --rounds 6(release builds of base and candidate, real servers over /ws)node drive.mjs <candidate-server> <scratch> releaseand the same against the base-commit server: 8 back-to-back?hot_reach(X, Y)reads over 100K edges with resident memory sampled every 20 ms from /procnode drive.mjs <candidate-server> <scratch> stream: 51 reads back to back for 12 s with resident memory samplednode drive.mjs <candidate-server> <scratch> guard-deadline:?hot_reach(X, Y)withtimeout_ms: 20after three large readsnode drive.mjs <candidate-server> <scratch> guard-memory INPUTLAYER_STORAGE__PERFORMANCE__MAX_QUERY_MEMORY_BYTES=33554432cargo test --lib protocol::handler::supervise(8 tests, including the two paused-time releaser tests)✅ No issues found.
?hot_reach(X, Y)over 100K edges and gets the answer faster than on the base commit, with the correct 1,002 rowsrecursive_query.latency_us.p50appears among the required metrics and gets a verdictperf-gate run --helpshowsgateas the default groupresource_exhausted, and the server keeps serving?edge(0, Y)answers in about 1 ms; guards-base.txt shows the same codes and comparable timingdeadline_exceededon time?hot_reach(X, Y)refused at 61-62 ms under a 60 ms deadline;?count(N)refused at 2002 ms under a 2 s deadline; server answers the next query; identical on basecargo test --lib protocol::handler::supervise, a unit test under paused Tokio time, which is not a run a…scripts/perf-gate.sh --baseline-rev c3d08ae5491824c3b21ba314ec9e38073845c66b --fixtures recursive_query --rounds 8 --server-cpus 8-31 --gate-cpus 0-7(builds both release servers, paired measurement, judged under perf-gate/policy.toml)node drive.mjs release <candidate inputlayer-server>: 100K-edge graph, one?hot_reach(X, Y)then quiet, then 12.5 s of back-to-back reads, resident memory sampled every 50 msnode drive.mjs release <base inputlayer-server>: the same sequence on the base commit for comparisonnode drive.mjs guards <candidate inputlayer-server>and the same on base:?hot_reach(X, Y)under MAX_QUERY_MEMORY_BYTES=32 MiB, MAX_TOTAL_QUERY_MEMORY_BYTES=32 MiB, QUERY_TIMEOUT_MS=60, and?count(N)of a recursion with no fixpoint under QUERY_TIMEOUT_MS=2000, each followed by a small querycargo test --lib protocol::handler::supervise(includes the two paused-time MemoryReleaser tests)perf-gate run --help: confirmsgateis the default fixture group, which now containsrecursive_query✅ No issues found.
?hot_reach(X, Y)over 100K edges repeatedly; the read is faster than at the base commit and returns the right 1,002 rowsscripts/perf-gate.sh --baseline-rev d5df1ba3 --fixtures recursive_query --rounds 6: p50 419.9 ms base vs 353.3 ms change, cost ratio 0.795 (interval 0.723..0.885); recursive-report.mdrecursive_querycase as a required metricrecursive_query.latency_us.p50under Required metrics with status PASS and row counts checked on every timed reply; recursive-report.mdresource_exhaustedand the server keeps serving?hot_reach(X, Y)refused after 69 ms at a 16 MiB limit,?edge(0, Y)answers afterwards; rss-cap-big.log: a cross join refused at the default 4 GiB limit with RSS peaking at 4.2 GiB?count(X)overcount(Y) <- count(X), Y = X + 1refused withdeadline_exceededafter 2002 ms atquery_timeout_ms = 2000; the next query answerscargo test --lib protocol::handler::supervise, a unit test under paused Tokio time, which is not the run…scripts/perf-gate.sh --baseline-rev d5df1ba3e25858c38a2f9c53a072c85e5d41e9d5 --fixtures recursive_query --rounds 6(release servers of base and change, paired and interleaved, standard profile)node drive.mjs <server> rss-quieton change and base: 8 serial?hot_reach(X, Y)reads over 100K edges, server RSS sampled every 25 ms during and for 4 s afternode drive.mjs <server> rss-capon change and base: 14 s of back-to-back reads, sudden RSS drops countednode drive.mjs <server> rss-cap-bigon change and base: a query run into the default 4 GiB limit, then 9 s of back-to-back reads, RSS per half secondnode drive.mjs <candidate> guard-memory:max_query_memory_bytesset to 16 MiB, then?hot_reach(X, Y)node drive.mjs <candidate> guard-deadline:query_timeout_msset to 2000, then a recursive counter rule with no fixpointcargo test --lib protocol::handler::supervise✅ **Document** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.
✅ No issues found.