Repository navigation
fix(protocol): judge a keyed subscription family while its views subscribe, so its first write shares one evaluation - #395
Conversation
…cribe, so its first write shares one evaluation (#383) A family of keyed subscriptions (one bound view per session, `?r1(H_k, Y)`) decides whether to share one evaluation per write by comparing a round of its lifted query with a view's own evaluation. A view's own cost counted only when its evaluation reused a compiled plan. Every view's text is distinct, so each first evaluation compiles its own plan, and a rule set keeps 256 plans: a family of 1,000 views that had only subscribed had no own cost, was never judged, and on its first write every view evaluated its own query, most of them compiling their plan again. On the views benchmark at 1M edges the issue's report measured about 740 ms of server CPU per write over the phase and an ack p99 of 680 ms. The cost of an evaluation has left out compiling its plan since rounds were judged that way (`CachedRun::executing`), so the restriction no longer protects the comparison. A view's own cost now counts every own evaluation: the family probes as soon as it has more bindings than compute permits, while its views subscribe, and its first write reads one round. Samples taken while sharing count too, so a family larger than the plan cache keeps its own cost current. Answers do not change: which evaluation a view reads was already a timing decision, and every view still evaluates its own query when it starts and after a rule change.
edd9a33 to
51cebe9
Compare
Measurements and local checksCompanion to the PR description's "Scope, measurements and known limits" and "Local checks" sections (the description is near GitHub's size limit, so the detail lives here). Benchmark host (sam-dev-benchmarks, 32 CPUs, servers on CPUs 8-31, each run alone under the host lock)
edd9a33 is the pre-rebase head; the current head 51cebe9 carries the same change rebased onto main (patch-ids identical in Perf gate
Local checks on 51cebe9 (GitHub CI is paused, #396)Shared devbox,
Failures, as they happened:
None of these is retried until green; they are reported as they are. |
Intent
Fix GitHub issue #383 in inputlayer/inputlayer (production readiness, performance): "Cold-start storm: a new family of 1,000 keyed views evaluates every own query on the first write (~745 ms CPU/write, ack p99 ~690 ms); plan cache (256) thrashes on 1,000 distinct keyed texts".
Found while fixing #378 (report on the benchmark host, ~/bench/fix-keyed-regress). Same on main and the fix. Live views (milestone 9, V11 #318) removes per-view evaluation; until then, consider judging a family before evaluating every own query, and keying the plan cache by lifted text.
Deliver it as a small PR; done means merged with CI green.
Substance of the referenced report (~/bench/fix-keyed-regress on the benchmark host, the views benchmark
perf-gate viewsat 1M edges): with 1,000 keyed subscriptions?r1(H_k, Y)subscribed as a new family and then written to, the first write evaluates every view's own query, so the keyed phase costs about 740 ms of server CPU per write with an ack p99 of about 680 ms and a write-to-delta p99 of about 750 ms, on both main and the #378 fix (PR #385); ramping the same family up through 1, 10, 100 then 1,000 subscribers instead costs 117 ms per write with a 4.3 ms ack p99. The server log of that run shows most own evaluations compiling their plan (plan_cached=false) because the 1,000 distinct keyed query texts do not fit the 256-plan cache.What Changed
Family::record_ownno longer skips evaluations that compiled their plan, and theplan_cachedfield is removed fromEvaluatedandQueryRowsinsrc/protocol/subscription/reevaluate.rs.stopped_at_bindingsbecomesjudged_at_bindings(recorded on every verdict, sharing or not), andstop_sharingcounts a failure only when the family has under twice the bindings of the previous verdict. A family with no counted failure since a round last kept sharing probes again after one commit's worth of own evaluations, rather than on every own evaluation.gui/lib/docs-bundle.ts) now says a failed try after the subscriptions doubled does not lengthen the wait.Risk Assessment
Testing
Drove the real release
inputlayer-serverwith the repository'sperf-gate viewsbenchmark at 1M edges, subscribing 1,000 keyed?r1(H_k, Y)views as one new family and then writing: once comparing baseline c3d08ae with candidate 51cebe9, and once more on the candidate alone for a second latency sample. Server CPU per write and rule evaluations per write show the fix in both samples; ack p99 shows it in the second sample only, the first being noisy under host load. The targeted keyed-subscription integration tests (5, against a real server over WebSocket) and theprotocol::subscription::parameterizedunit tests (33, including the growing-family and backoff cases) passed on the target commit; that run was launched by the previous timed-out attempt and finished during this one. The unit tests are in-process and do not count as live validation, so the growing-family re-judging scenario is reported as untested. No UI surface is involved, so there is no visual evidence. Benchmark data directories were removed and the worktree is clean.standing_query_tests parameterized, 5 passed, including bound_subscriptions_share_one_evaluation_and_get_only_their_rows and every_subscriber_follows_its_own_query_through_mixed_…cargo test --all-features --lib protocol::subscription::parameterized, 33 passed, including a…Evidence: Cold 1,000-view family at 1M edges, baseline vs candidate (this attempt)
| phase | subs | ack p50 / p99 ms | write->delta p50 / p99 ms | server CPU ms/write | frames/write | rule evaluations/write | late | |---|---|---|---|---|---|---|---| | baseline c3d08ae5 P4_r1_keyed_x1000 | 1000 | 144.5 / 268.3 | 238.4 / 362.1 | 1292.0 | 1001.0 | 102.0 | 0 | | candidate 51cebe99 P4_r1_keyed_x1000 | 1000 | 125.8 / 331.2 | 249.5 / 425.3 | 123.0 | 1001.0 | 2.0 | 0 | Host load average 17-30 during the run; 10 writes per phase, so p99 is the maximum.Evidence: Cold 1,000-view family, candidate only, second sample (this attempt)
| phase | subs | ack p50 / p99 ms | write->delta p50 / p99 ms | server CPU ms/write | frames/write | rule evaluations/write | late | |---|---|---|---|---|---|---|---| | candidate 51cebe99 P4_r1_keyed_x1000 | 1000 | 6.3 / 10.2 | 101.0 / 112.6 | 135.5 | 1001.0 | 2.0 | 0 | 20 writes; host load average 27-34.Evidence: Re-run benchmark logs and raw results
Evidence: Raw result JSON, baseline vs candidate re-run
Evidence: Targeted keyed-subscription tests on 51cebe9
~/.no-mistakes/evidence/01M47ATDCGNKDHA3TX4N287PH6/views-cold-1000-summary.md)~/.no-mistakes/evidence/01M47ATDCGNKDHA3TX4N287PH6/views-ramp-compare-summary.md)Scope, measurements and known limits
Part of #383: this PR addresses the first half only, the cold-start storm, not the plan cache (
MAX_CACHED_PLANS = 256, keyed by query text; a lifted-text key needs parameterised plans). Benchmark host,bench-views.sh --edges 1000000 --keyed 1000, 1,000 keyed views as a new family then 20 writes: main a649cd9 663.5 ms server CPU/write, ack p99 598.2 ms, 52 rule evaluations/write; this PR 108.0 ms, 4.1 ms, 2.0. Remaining plan-cache work: a family of more than 256 views still recompiles most plans on writes where its views evaluate their own queries: (1) after a failed probe, until it probes again; (2) when a parameter binds a recursive relation, never shared by design; (3) on the first write after a rule change. Known limit: a failed verdict is uncounted only while bindings have at least doubled since the previous verdict; a write landing mid-ramp before that counts its failure, and the family then waits the normal 256 writes to be judged again at its settled size, as main did for a family judged on its first write. Perf gate: all required metrics PASS exceptcheap_query.queries_per_secINCONCLUSIVE (1.044, budget 1.05), which passes against main (0.994): not this PR's. Tables and gate reports: PR comment "Measurements and local checks".Local checks
GitHub CI is paused (#396). Run on commit 51cebe9 (the PR head), shared devbox at load 15-56, debug info off, full suites one at a time under
flock; loglocal-checks-51cebe99.login the task data directory, details in the PR comment.make fmt-checkmake lintmake unit-testsync_replication_tests::async_mode_never_tracks_or_waits,sync_replication_tests::a_write_is_acknowledged_once_a_follower_confirms_its_events,handler::tests::test_execute_program_touches_session_on_non_query,residency::tests::a_knowledge_graph_with_state_only_in_memory_stays_loaded(all 4 pass rerun alone)make e2e-testmake oracle-testmake e2e-reactivesaturation::saturated_sessions_get_every_delta_once_in_time_and_nothing_else(delta 7.4 s > 5 s bound; same failure alone and on origin/main d5df1ba at this load)make python-testmake js-testmake perf-gate-check,make pre-pr-selftestPipeline
Updates from git push no-mistakes
... (16 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
The intent words this as "consider", not as required, so this is not a contradiction, but merging will close #383 with that half open. Decide whether to keep it in scope, or split it into a follow-up issue before closing. Fixing it would extend the change (a new cache key or parameterised plans), so the remedy needs authorization rather than an auto-fix.
src/protocol/subscription/parameterized.rs:811- Introduced by the round 1 fix: a family that fails its probes while views keep joining now evaluates its lifted query on every commit, where it used to back off 256 commits after the first failure. Traced from source, not measured.Sequence: a non-sharing family of n views whose lifted query is too slow, with at least one new binding joining between consecutive commits (a burst of subscriptions during ongoing writes, or users connecting steadily). On each commit the n own evaluations bring
own_since_stopup towait = bindings(line 842-846), so a probe starts. It fails, andstop_sharingseesbindings > judged_at_bindings(line 811), sostopsstays 0 and the wait stays one commit. This repeats for as long as growth continues, and each probe is a full lifted evaluation (about 117 ms at 1M edges per the issue). Example: 1,000 views subscribing over 5 s at 100 writes/s gives up to about 500 probes, against about 7 under the doubling rule alone.The cost is bounded: probes run one at a time under the probe permit, no view waits for them, and the loop ends at the first commit interval with no join. The same path applies when a round errors rather than being too slow (
stop_sharingcalls at lines 714 and 721).The cause is that 'still growing' is defined as any increase since the last verdict, even one binding. A narrower definition that still satisfies the operator's decision: count a failure unless bindings have at least doubled since the last verdict, the same threshold as
grownat line 837. For the 9 -> 1,000 sequence, the verdicts at 9, 18, ... 576 stay uncounted, the family is judged once at 1,000 after one commit, and that failure counts. Probes during growth are then bounded by log2 of the growth plus one. This would change the last steps offailures_while_a_family_grows_do_not_lengthen_its_wait, which expects two verdicts at 1,000 beforestopsbecomes 1.This is a policy choice about what 'settled' means, so it needs the operator rather than an auto-fix.
src/protocol/subscription/parameterized/tests.rs:669- Added in the round 1 fix:a_family_too_slow_to_share_while_its_views_subscribe_shares_at_full_sizesets the own cost to 1/32 of one measured round, then relies on two later probe rounds landing within 8x of that measurement. The 9-view probe must take more than 4x own (round/8), and the 1,000-view probe at most 250x own (about 7.8x round). The round is a millisecond-scale query over 1,000 rows, so one scheduling stall on a loaded CI host during the measured round or the second probe can flip either assertion. The operator asked for no extreme own cost, so this is the accepted tradeoff; noted in case the test flakes in CI.🔧 Fix applied.
5 issues (4 warnings, 1 info) still open:
src/protocol/subscription/parameterized.rs:836- Judging a family while it is still growing can leave it not sharing at its final size, with a much longer retry backoff than before. Not measured: this is traced from source, and whether it bites depends on the round/own cost ratio on the benchmark host.Concrete sequence (issue scenario, 1,000 views subscribed one after another, 8 compute permits):
record_ownnow returns true on the first own evaluation with 9 bindings, so the probe is judged bykeeps_sharingat 9 bindings, where a round may be at most 4x a view's own cost. A lifted?r1(_L0, Y)over 1M edges fails that, sostop_sharing(line 798) setsstops=1,stopped_at_bindings=9. Thegrownrule then re-probes at roughly 18, 36, 72, 144, 288 and 576 bindings, each failure incrementingstops. No further growth probe is possible before 1,152 bindings, so the last verdict is the one at ~576, where the threshold is 144x own cost; at 1,000 bindings it would be 250x. A family whose round costs between those two is left not sharing, although it would share if judged at full size.Once stuck, the next probe needs
probe_after(1000, 7)= 256 * 1000 * 64 own evaluations, about 16,384 writes each evaluating all 1,000 views (the #383 storm). Before this change, a family subscribed in one go was first judged on its first write at full size withstops == 0, and a failure there backed off 256 writes. The issue's own numbers (about 117 ms per shared write against sub-millisecond own evaluations) put the ratio in the low hundreds, near this band.Same class, other sites:
stop_sharingat line 798 counts subscribe-time failures like steady-state ones;stopsis only reset on a successful round (line 742) or a rule change (line 612). The new unit testa_family_judged_while_its_views_subscribe_shares_its_first_commitforcesown_cost_usto 1 s, so it does not exercise a failing subscribe-time probe followed by growth.The remedy is a policy choice, so it needs the author: for example, not counting a failure (or not growing the backoff) for a verdict reached while the family is still growing, or re-judging once bindings settle. Benchmark confirmation on
perf-gate viewsat 1M edges would settle whether the band is hit in practice.src/storage_engine/snapshot.rs:123- The change addresses only the first of the two things the issue names. The intent's title includes "plan cache (256) thrashes on 1,000 distinct keyed texts" and suggests to "consider ... keying the plan cache by lifted text"; the diff leavesMAX_CACHED_PLANS = 256and the per-text plan key (src/protocol/handler.rs:3729-3736) untouched.The thrash stays reachable whenever a family of more than 256 keyed views evaluates its own queries on a write: when its probe fails (see the other finding), when a parameter binds a recursive relation (never shared, by design), and on the first write after a rule change, which resets sharing and empties the plan map. In those cases most of the 1,000 own evaluations still compile their plan on every write.
The intent words this as "consider", not as required, so this is not a contradiction, but merging will close #383 with that half open. Decide whether to keep it in scope, or split it into a follow-up issue before closing. Fixing it would extend the change (a new cache key or parameterised plans), so the remedy needs authorization rather than an auto-fix.
src/protocol/subscription/parameterized.rs:811- Introduced by the round 1 fix: a family that fails its probes while views keep joining now evaluates its lifted query on every commit, where it used to back off 256 commits after the first failure. Traced from source, not measured.Sequence: a non-sharing family of n views whose lifted query is too slow, with at least one new binding joining between consecutive commits (a burst of subscriptions during ongoing writes, or users connecting steadily). On each commit the n own evaluations bring
own_since_stopup towait = bindings(line 842-846), so a probe starts. It fails, andstop_sharingseesbindings > judged_at_bindings(line 811), sostopsstays 0 and the wait stays one commit. This repeats for as long as growth continues, and each probe is a full lifted evaluation (about 117 ms at 1M edges per the issue). Example: 1,000 views subscribing over 5 s at 100 writes/s gives up to about 500 probes, against about 7 under the doubling rule alone.The cost is bounded: probes run one at a time under the probe permit, no view waits for them, and the loop ends at the first commit interval with no join. The same path applies when a round errors rather than being too slow (
stop_sharingcalls at lines 714 and 721).The cause is that 'still growing' is defined as any increase since the last verdict, even one binding. A narrower definition that still satisfies the operator's decision: count a failure unless bindings have at least doubled since the last verdict, the same threshold as
grownat line 837. For the 9 -> 1,000 sequence, the verdicts at 9, 18, ... 576 stay uncounted, the family is judged once at 1,000 after one commit, and that failure counts. Probes during growth are then bounded by log2 of the growth plus one. This would change the last steps offailures_while_a_family_grows_do_not_lengthen_its_wait, which expects two verdicts at 1,000 beforestopsbecomes 1.This is a policy choice about what 'settled' means, so it needs the operator rather than an auto-fix.
src/protocol/subscription/parameterized/tests.rs:669- Added in the round 1 fix:a_family_too_slow_to_share_while_its_views_subscribe_shares_at_full_sizesets the own cost to 1/32 of one measured round, then relies on two later probe rounds landing within 8x of that measurement. The 9-view probe must take more than 4x own (round/8), and the 1,000-view probe at most 250x own (about 7.8x round). The round is a millisecond-scale query over 1,000 rows, so one scheduling stall on a loaded CI host during the measured round or the second probe can flip either assertion. The operator asked for no extreme own cost, so this is the accepted tradeoff; noted in case the test flakes in CI.src/protocol/subscription/parameterized.rs:812- In the round 2 fix: an uncounted verdict keeps an earlier counted failure, so a family that receives one write while its views are still subscribing is not re-judged after one commit at its settled size. Traced from source, not measured.Sequence (8 permits, lifted query too slow for small sizes): the probe at 9 bindings fails uncounted (
stops=0,judged_at_bindings=9). Three more views subscribe, then a write lands at 12 bindings: its 12 own evaluations bringown_since_stopto 15 >=wait(12, line 843-847), a probe starts and fails, and 12 < 18 so it counts (stops=1). Growth continues: the verdicts at 24, 48, ... 768 are uncounted but leavestopsat 1. At 1,000 bindingsgrownis false (needs 1,536) andwaitisprobe_after(1000, 1)= 256,000 own evaluations, i.e. 256 writes each evaluating all 1,000 views. The last verdict was at 768 bindings (round allowed 192x own cost) where 1,000 allows 250x.This contradicts the operator's round 2 decision ("a family is judged for the record after one commit at its settled size") and the comments that state it unconditionally: module doc lines 62-65 ("a family that stopped with at least twice the bindings of its previous verdict probes again after one commit") and the
record_owndoc at lines 818-823. It only holds when no failure was counted earlier. Without a write during the ramp the behaviour is as decided.This is a narrower remainder of the ignored finding subscribe-time-probe-failures-accumulate, left behind by the round 2 fix; it is no worse than base for a family that also fails at full size (base also waited 256 writes). Both new tests (
failures_while_a_family_grows_do_not_lengthen_its_wait,a_family_gaining_a_view_per_commit_does_not_probe_on_every_commit) pass with it: neither has a commit between doublings followed by further doubling.Possible remedy: an uncounted stop also resets
stopsto 0 (line 812-814), so the one-commit wait applies after every doubling; probes during growth are then bounded by about 2 per doubling. Alternatively keep the behaviour and correct the two comments. This is a policy choice about the backoff, so it needs the operator.🔧 Fix applied.
2 issues (1 warning, 1 info) still open:
src/storage_engine/snapshot.rs:123- The change addresses only the first of the two things the issue names. The intent's title includes "plan cache (256) thrashes on 1,000 distinct keyed texts" and suggests to "consider ... keying the plan cache by lifted text"; the diff leavesMAX_CACHED_PLANS = 256and the per-text plan key (src/protocol/handler.rs:3729-3736) untouched.The thrash stays reachable whenever a family of more than 256 keyed views evaluates its own queries on a write: when its probe fails (see the other finding), when a parameter binds a recursive relation (never shared, by design), and on the first write after a rule change, which resets sharing and empties the plan map. In those cases most of the 1,000 own evaluations still compile their plan on every write.
The intent words this as "consider", not as required, so this is not a contradiction, but merging will close #383 with that half open. Decide whether to keep it in scope, or split it into a follow-up issue before closing. Fixing it would extend the change (a new cache key or parameterised plans), so the remedy needs authorization rather than an auto-fix.
ℹ️
src/protocol/subscription/parameterized/tests.rs:669- Added in the round 1 fix:a_family_too_slow_to_share_while_its_views_subscribe_shares_at_full_sizesets the own cost to 1/32 of one measured round, then relies on two later probe rounds landing within 8x of that measurement. The 9-view probe must take more than 4x own (round/8), and the 1,000-view probe at most 250x own (about 7.8x round). The round is a millisecond-scale query over 1,000 rows, so one scheduling stall on a loaded CI host during the measured round or the second probe can flip either assertion. The operator asked for no extreme own cost, so this is the accepted tradeoff; noted in case the test flakes in CI.ℹ️
gui/lib/docs-bundle.ts:1- The regenerated docs bundle (commit 51cebe9) also picks up replication-guide text unrelated to this change (the 64 MiB follower buffer, follower pings, removal of the Warm standby: follower must keep reading the stream while it reconciles a large graph #356 known limit). The bundle was stale at the base commit: the new text matchesdocs/content/docs/guides/replication.mdxat HEAD, so the regeneration is faithful and nothing is wrong in it. Noted only so the extra lines in the PR diff are not a surprise.✅ No issues found.
?r1(H_k, Y)as a new family on a 1M-edge graph, then write: the writes cost one shared evaluation instead of 1,000 own evaluationsscripts/bench-views.sh --baseline-rev 00565385 --edges 1000000 --keyed 1000 ...(cold-1000-summary.md). Phase P4_r1_keyed_x1000: base 689.5 ms CPU/write, ack p50/p99 266.4/564.5 ms, write-to-delta p…snapshot_execute_cachedlines over the whole run: base 2,125 executions of which 1,752 compiled a plan; t…scripts/bench-views.sh --baseline-rev 00565385 --edges 100000,1000000 --keyed 1,10,100,300,1000 ...(ramp-summary.md). At 1M edges and 1,000 subscribers: base 118.0 ms CPU/write, ack p99 6.8 ms; tar…?hot_reach(H_k, Y)(more views than compute permits) are never served by a lifted evaluation and still get every deltacargo test --test standing_query_tests parameterized(5 passed, including `bound_sub…cargo test --lib protocol::subscription::parameterized(33 passed, including `failures_while_a…cargo test --lib protocol::subscription::parameterized(33 passed)cargo test --test standing_query_tests parameterized(5 passed)scripts/bench-views.sh --baseline-rev 0056538513506df1f691369e28737e5ff6534436 --edges 1000000 --keyed 1000 --large-recursive-keyed 1 --reads 1 --filler-rules 0(cold start, both servers, exit 0)scripts/bench-views.sh --baseline-rev 0056538513506df1f691369e28737e5ff6534436 --edges 100000,1000000 --keyed 1,10,100,300,1000 --recursive-keyed 100 --large-recursive-keyed 1 --reads 1 --filler-rules 0(ramp-up and recursive keyed, both servers, exit 0)Countedsnapshot_execute_cachedlines withplan_cached=true/falsein both servers' logs from the cold-start runDisposable Rust driver reusingperf-gate/src/client.rsandserver.rsagainsttarget/release/inputlayer-server: 1,000 views on 300K edges and 200 views on 60K edges subscribing while a writer inserts and retracts, each view checked against an ad-hoc read and the writer's recordRemoved the driver, the base build and the benchmark outputs fromtarget/;git statusis clean and no server I started is left running🔧 No changes applied.
1 info still open:
make bench-views-remote) would give clean latency numbers if they are wanted for the PR.standing_query_tests parameterized, 5 passed, including bound_subscriptions_share_one_evaluation_and_get_only_their_rows and every_subscriber_follows_its_own_query_through_mixed_…cargo test --all-features --lib protocol::subscription::parameterized, 33 passed, including a…target/release/perf-gate views --server target/release/inputlayer-server --compare target/perf-gate/servers/c3d08ae5…/inputlayer-server --edges 1000000 --keyed 1000 --large-writes 10 --reads 2 --large-recursive-keyed 1 --filler-rules 50(baseline vs candidate, exit 0)target/release/perf-gate views --server target/release/inputlayer-server --edges 1000000 --keyed 1000 --large-writes 20 --reads 1 --large-recursive-keyed 1 --filler-rules 50(candidate only, second sample, exit 0)cargo test --all-features --test standing_query_tests parameterized(5 passed)cargo test --all-features --lib protocol::subscription::parameterized(33 passed)Read the earlier evidence from this step on the same commit (views-cold-1000-summary.md,views-ramp-compare-summary.md) for the ramp comparison; not re-driven in this attempt✅ **Document** - passed
✅ No issues found.
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.