Repository navigation
fix(protocol): keep keyed subscription families sharing when sharing saves most of the work (#378) - #385
Merged
Conversation
…e work, and judge again as views join (#378) A family of keyed subscriptions (one bound view per session, `?r1(H_k, Y)`) shared one evaluation per write only while a round was no slower than the views' own evaluations run in parallel on the compute permits. That compares latency on an idle server and ignores the work: with 18 permits a family accepted up to 18 times the CPU for equal latency, and every own evaluation holds a permit that writes and reads wait for. #304 made bound rule reads about five times cheaper (`?r1(400,Y)` at 100k edges: 6.3 to 1.2 ms). Own evaluations then looked faster in parallel than a round, so keyed families stopped sharing (logged on main at 1M edges: `shared_us=85445 own_us=9739 bindings=100 permits=18`), and the backoff after that verdict outlasted the 1,000 views that joined later. On the views benchmark a write with 100 keys at 100k edges cost 154 ms of server CPU instead of 46, and with 1,000 keys at 1M edges 11.9 s and a 643 ms ack instead of 0.18 s and 3.8 ms. - A round now also keeps sharing when it does at most half the views' own evaluations' combined work. - A family that stopped sharing probes again as soon as it has twice the bindings it had when it stopped, instead of waiting out a backoff that grows with the bindings: a verdict reached while the first views subscribed no longer holds for the many that follow. On the benchmark host (servers on 24 CPUs, 18 compute permits) the issue's case, 1M edges with keyed 1, 10, 100 then 1,000 subscribers, now costs 117 ms of server CPU per write at 1,000 keys, with a 4.0 / 4.3 ms ack p50/p99 and a 75 / 79 ms delta p50/p99 (main: 11,812 ms, 649 / 687 ms, 689 / 1,212 ms; before #304: 180 ms, 3.8 / 8.3 ms, 75 / 104 ms). At 100 keys on 100k edges: 17 ms per write (main 154, before #304 46).
… CPU per write (#378) `delta_keyed`: 300 agents, each subscribed to its own key `?r1(H_k, Y)` of a deployed rule over 100K edges; an open-loop writer adds one row to one key every 20 ms. Every write refreshes all 300 views, which differ only in their constant, so the engine must evaluate the family once per write rather than each view's own query. Besides delta and ack latency, the fixture reports `writes_per_server_cpu_sec`, writes per second of server CPU over the write phase, which shows lost sharing even on a host idle enough for the deltas to arrive in time. Its delta p50/p99, ack p99 and CPU rate join the policy's required set, and the fixture joins the gate's default fixtures. Six rounds on the benchmark host. Against the commit before #304 (6ac5322), every required metric passes: delta p50 10.5 ms on both, delta p99 29.7 to 16.6 ms, ack p99 20.3 to 9.6 ms, 2.3 times less server CPU per write. Against current main the fixture shows the trade the sharing fix makes, which is accepted: 6 to 8 times less server CPU per write and a better tail under load (at a write every 10 ms main's delta p99 reaches 51 ms, the fix's stays under 19 ms), for a higher idle delta p50 (5.8 to 10.2 ms), because main evaluates all 300 views in parallel at that CPU cost.
…stfmt keeps_sharing
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
I want you to really hyper focus on making the symbolic engine, you know, crisp and really truly production ready. (Context: #378 : a write at 1M edges with 1,000 keyed subscriptions now costs 11.9 s CPU and a 643 ms ack, against 0.18 s and 3.8 ms before. Measured by the views benchmark from PR #369 (branch fm/m9-v1-benches, make bench-views-remote).)
Substance of the referenced issue #378 and benchmark: the views benchmark (PR #369, perf-gate views) reproduces the architecture review's baseline on the review's commit within noise, but current main regressed keyed rule-view sharing: N subscribers each subscribed to their own key ?r1(head_i, Y) of the deployed rule r1(X,Y) <- edge(X,Y), label(X,"hot"), each write changing one key's result; at 1M edges with 1,000 keys one write costs 11.9 s CPU and a 643 ms ack, against 0.18 s and 3.8 ms before. Suspects merged since: #355 (session throughput, round dedup), #303 (shared probe round fix), #282 (parameterised programs). Bisect on the benchmark host, then fix or revert the culprit; add this case to the perf gate so it can never regress silently again.
What Changed
src/protocol/subscription/parameterized.rs,keeps_sharingnow has a second way to accept a shared round. Before, a round passed only if it was no slower than the views' own evaluations running in parallel on the compute permits. Now it also passes if it does at most half of the views' combined work and is at most twice as slow as those parallel evaluations. When a family stops sharing, it records its binding count, and it probes again as soon as that count doubles, so a verdict reached while views were still subscribing is not kept for the many that join later. New unit tests cover the new thresholds, overflow, the Regression: keyed rule-view sharing, a write costs 11.9 s CPU and a 643 ms ack at 1M edges / 1,000 keys (was 0.18 s / 3.8 ms) #378 shape (100 keys, 18 permits, 7 ms round), and probing again once bindings double.delta_keyedperf-gate fixture (perf-gate/src/fixtures/keyed.rs, plus profile params and aGraph::chainsdataset). It runs 300 agents, each subscribed to its own key?r1(H_k, Y)ofr1(X, Y) <- edge(X, Y), label(X, "hot")over 100K edges, and an open-loop writer whose writes each change one key.policy.tomlnow requiresdelta_usp50/p99,ack_usp99 and a newwrites_per_server_cpu_secmetric. The gate's default fixtures go from eight to nine.config.example.tomland the GUI docs bundle to describe the new sharing criteria and the re-probe on doubled bindings.Bisect and benchmark results
All runs on the benchmark host sam-dev-benchmarks (nproc 32, servers pinned to CPUs 8-31, 18 compute permits), one at a time under the shared lock, with the views benchmark from #369.
Culprit: #304 (0341d8c). Keyed x100 at 100k edges, server CPU per write:
#304 made the bound read
?r1(400,Y)about five times cheaper (6.3 to 1.2 ms). The family's latency-only judge then refused to share (logged:shared_us=85445 own_us=9739 bindings=100 permits=18), and its backoff outlasted the 1,000 views that joined later. #304 itself is kept; the judge is fixed.Issue #378's case (1M edges, keyed 1, 10, 100 then 1,000), keyed x1000:
Keyed x100 at 1M: 1,040 ms (main) to 127.5 ms CPU per write. Keyed x100 at 100k: 134.7 to 16.7 ms.
New
delta_keyedgate fixture, 6 paired rounds against main: server CPU per write 85 ms to 16-24 ms (PASS); ack p50/p99 equal within noise; all 12 runs delivered every one of the 300 agents exactly its own rows.Accepted trade (firstmate's decision): against current main,
delta_keyeddelta p50 rises from about 5.3 to 10.6 ms and p99 from about 11.7 to 16.6 ms (both FAIL), because main evaluates all 300 views in parallel at 6-8 times the CPU. Sharing again costs 3.5-8 times less CPU per write, keeps the tail bounded under load, and matches the delta latency of the commit before #304 (10.5 ms p50 on both). It is capped: a family shares for the work it saves only while a round is at most twice as slow as the views' own evaluations in parallel.Not fixed here: a fresh family of 1,000 views still re-evaluates every view on its first write before any verdict (cold-start storm, about 745 ms CPU per write averaged over 20 writes at 1M edges, on main and here alike); #304 also made recursive reads 9-20% slower; the approved perf-gate baseline (e732cd5) predates subscription families, so
delta_keyedcatches sharing regressions only against a merge base.Risk Assessment
✅ Low: The fix round matches the recorded decision.
keeps_sharingnow computesshared_us <= min(max(parallel, combined), 2*parallel)with saturating arithmetic. Traced inputs: the 1,000 x 1 ms example on 18 permits gives 112,000 (shares) and 112,001 (stops), so 450 ms stops. The #378 cases still share: 7 ms against 12 ms, and the logged 1M-edge case of 85,445 us against 116,868 us. The overflow assertions also hold. The re-probe when bindings double resets on a rule change and is refreshed at each stop, and the docs and docs bundle describe the new rule consistently.Testing
As the user decided, this phase records the locked benchmark-host job for exactly head 1b215441 against base 67a9782f as its live validation. It did not rebuild servers or re-run the perf gate. I confirmed the raw results on the host and copied the reports into the evidence directory. - Perf gate, delta_keyed: all 12 runs exited cleanly and delivered every agent's own rows. CPU per write passes at a cost ratio of 0.28. Delta p50 and p99 are FAIL against base: that is the higher idle delta the user accepted in exchange for the CPU savings, now capped at twice the parallel own latency. Ack is INCONCLUSIVE (within noise). - Views benchmark at 1M edges: keyed x1000 server CPU per write falls from 11,297 ms to 119.5 ms. Ack p99 falls from 624 ms to 4.5 ms, and delta p99 from 1,104 ms to 80 ms. Base logs the family stopping sharing; the candidate never does. - Views benchmark at 100k edges: keyed x100 CPU per write falls from 134.7 ms to 16.7 ms. The parameterized-subscription unit tests, including the new 1,000 × 1 ms on 18 permits latency-ceiling case, pass. There is no UI surface, so there are no screenshots. The worktree is clean.Evidence: perf-gate delta_keyed paired report (base 67a9782 vs candidate 1b21544)
Evidence: perf-gate delta_keyed verdict
Evidence: perf-gate run metadata (host, CPUs, command, exit)
Evidence: Views benchmark 1M edges, candidate
Evidence: Views benchmark 1M edges, base
Evidence: Views benchmark 100k edges, candidate
Evidence: Views benchmark 100k edges, base
Evidence: Keyed x1000 at 1M edges, base -> candidate
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
src/protocol/subscription/parameterized.rs:110- The new work criterion inkeeps_sharing(shared_us <= max(parallel, own*bindings/2)) puts no ceiling on how much slower a shared round can be than the views' own evaluations run in parallel. Example: 1,000 keyed views at 1 ms each on 18 permits. Parallel own evaluations finish in about 56 ms. The work bound is 500 ms, so a 450 ms round keeps sharing. Every delta in the family then waits roughly 8 times longer while saving only about 55% of the CPU. The commit message accepts a version of this trade (idle delta p50 5.8 to 10.2 ms on delta_keyed). The unbounded case is still a product decision: should sharing also require the round to stay within some multiple of the parallel latency, or should the divisor depend on load? It is flagged because the intent asks for a production-ready engine, and this changes latency behaviour for every keyed family, not only the benchmark's.🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Test** - 2 issues found → no changes applied ✅
make perf-gate-remoteon the benchmark host, or the local delta_keyed perf-gate run, to confirm the CPU per write and the latency before shipping.scripts/perf-gate.sh --baseline-rev 67a9782fef6d18f36229668e4fbe6392871c0330 --fixtures delta_keyed --rounds 3 --no-verdict --server-cpus 8-31 --gate-cpus 0-7(started; stopped while compiling the base commit's server; no measurement)Read the diff between 67a9782f and 1b215441:keeps_sharingwith SHARED_WORK_DIVISOR and the SHARED_SLOWDOWN latency cap, re-probing once bindings double, and the new delta_keyed fixture added to the policy's required set🔧 No changes applied.
✅ Re-checked - no issues remain.
Read-only inspection of the user-recorded locked benchmark job on sam-dev-benchmarks (~/bench/fix-keyed-regress/results/{ev-gate,e1m,e100}), confirming it ran exactly candidate 1b215441 against baseline 67a9782fperf-gate-1b215441 run --fixtures delta_keyed --rounds 6 --server-cpus 8-31(ev-gate report.md / verdict.json / summary.md / bench.txt): 300 keyed agents, 12 runs, exit 0Views benchmark at 1M edges, keyed x1/x10/x100/x1000, base vs candidate (e1m summaries) plus a count ofsubscription_family_stops_sharingin each server logViews benchmark at 100k edges, keyed x100, base vs candidate (e100 summaries)cargo test --lib subscription::parameterized(29 tests, including the new latency-ceiling test a_round_saving_work_shares_while_at_most_twice_as_slow_as_the_views_in_parallel)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.