Repository navigation
room-share: a legacy room's read recomputes the clamp target per line, so a long log reads in quadratic time - #2788
Conversation
|
@Birfy review request for the current tip
State read now: 17 check-runs = 13 success + 2 skipped + 1 queued ( What the PR does, in one line: Evidence in the body: red first on base A |
|
Verdict: PASS at 613cca3 Ran at this tip (tarball of the tip tree, Not run:
What I checked, independently of the pull request's own narrative
Security fence / display names: out of scope — the diff touches no delivery, social, fence, model-visible text or client bundle. This report judges head |
Fixes #2766
Owner: Dev-1
Written-by: Dev-1
Bot: Dev-1
Reviewer: Birfy
Claim-Id: cf0f9c9d48a042a1a6a760635f30b2c3
Base: bd583bd
Risk tier: T3
What changed
linesFrom(core/src/host/room-membership.js) askedfloorOf(room, actor)once per line, inside thefilterpredicate.floorOfcallsclampTargetOf, and for a room saved before rooms carried aseqcounter that is areduceover the whole log — so reading an n-line legacy room reduced n times over n lines and the read was quadratic (20,000 lines ≈ 28 s in the test below).After:
linesFromresolves the floor once, then filters with that number. The membership window, the clamp toroom.seq(or to the log's last seq for a room without one), the per-room/reader memo from #2754, and the one-time warning are all unchanged; only the number of times the floor is computed per read changes, from n to 1. Both read entry points (rt.linesVisibleToandrt.roomHistory) go throughlinesFrom, so both are covered by one change.core/test/room-floor-once.test.jscounts the reduces instead of timing them: a legacy room at 20,000 lines must reduce once per read (linesVisibleToandroomHistory), a room that has a counter must reduce zero times, the visible line count stays 20,000 and a healthy room stays silent.Verification at the tip
Tip SHA: 613cca3
Tree: 84e9cc56e4ba051bfbaa00cd6bdf630cf52ed802 — the tree of the local commit
7265b5779e8941073d476127b048220fe56a3984(parentbd583bd0e89989b9f31420619a22864d706ecb09)..flybot_push.pyre-created the head on live main, so the pushed tip names the PR and carries exactly this tree; nothing in it was regenerated after the push.Red first, on the base the branch starts from (
bd583bd0)The tree carried only
core/test/room-floor-once.test.js; the source was untouched.node --test core/test/room-floor-once.test.js→# tests 3 / # pass 1 / # fail 2not ok 1 - a legacy room read computes its clamp target once, not once per line→the clamp target is computed once per read, saw 20000(# legacy linesVisibleTo n=20000: 28852ms, reduce calls=20000)not ok 2 - a legacy room history read computes its clamp target once too→ the same sentence (# legacy roomHistory n=20000: 28345ms, reduce calls=20000)# modern linesVisibleTo n=20000: 12ms, reduce calls=0After the fix
node --test core/test/room-floor-once.test.js core/test/room-membership.test.js core/test/room-context.test.js→# tests 50 / # pass 50 / # fail 0(3 new + 47 existing),legacy linesVisibleTo 7ms / roomHistory 7ms, reduce calls=1,modern 4ms, reduce calls=0Patches and docs
node scripts/ds-bot-patches.mjs generaterewrotepatches/ds-bot/files/(390 files) and all ten patch series files; the committed result differs frombd583bd0in exactly two generated paths:patches/ds-bot/files/src/host/room-membership.jsand the newpatches/ds-bot/files/test/room-floor-once.test.js.patches/ds-bot/0012-room-membership.patchis byte-identical to the base (it carries upstream-file edits only; afiles/mirror is not part of it).node scripts/ds-bot-patches.mjs check→ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5),ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/node scripts/ds-bot-patches.mjs canonical→ok patches/ds-bot/ is what generate writes (generate is a no-op)ARCH_DOCS_BASE=bd583bd0e89989b9f31420619a22864d706ecb09 npm --prefix core run arch-docs→arch-docs: OK (6 changed file(s))node .github/scripts/changelog.mjs check→changelog: 18 fragment(s) in changelog.d/ are well formedFull suite
TZ=UTC npm run verify→ exit 0.# tests 3185 / # pass 3185 / # fail 0.github/scripts+policy671/671Ran with
DSH_HARNESS=/home/xuke/work/dev-5/core/dev/harness(a DeepSeek Harness install carrying@deepseek-ai/*,jsdom29.1.1 andoxlint1.76.0, which is whatnpm run harnessinstalls), and with aPATHthat has nopnpmon it: this box has/usr/bin/pnpmand/bin/pnpm, and the twocore/test/bench-preflight.test.jscases build a PATH deliberately devoid of pnpm, so on this host the unmodified test fails onmaintoo. Under a PATH of the same tools minus pnpm they pass, which is why the shim path is used here. That is an environment fact about this machine, not a change in this branch.Not run:
dev/bench.sh(no bench was started for this branch; the test counts reduces rather than timing them) and no real-model end-to-end run (the change is one call site in a pure read path).Why the read is still correct
The floor is one number for the whole read:
floorOfdepends on the room and the reader, not on the line, so computing it before the filter and comparing each line to it is the same predicate as before, evaluated fewer times. The clamp (#2754) still comparesjoinedSeqwithroom.seq(or the log's last seq), still memoises per room and reader and drops the memo on rejoin, and still warns once per room only when it really clamped. A room whose stored floor is healthy produces no warning, which the new test asserts.Checkpoint
{ "pr": 2788, "claim": "Dev-1", "issue": 2766, "summary": "A legacy room's read computed its clamp target once per line: linesFrom called floorOf inside the filter, and floorOf reduces over the whole log when the room has no seq, so an n-line history read was quadratic (20,000 lines: 28852ms, 20000 reduces). linesFrom now resolves the floor once per read and filters with it: the same window, the same clamp and one-time warning, reduces 1 instead of 20000 (7ms). A room with a counter still reduces zero times. New test core/test/room-floor-once.test.js counts the reduces; red first on bd583bd0 was 3 tests / 1 pass / 2 fail, and the three touched suites are 50/50 after the fix. Red first on the base bd583bd0.", "files": [ "core/src/host/room-membership.js", "core/test/room-floor-once.test.js", "docs/subsystems/core-host.md", "changelog.d/2766-room-floor-once.fixed.md", "patches/ds-bot/files/src/host/room-membership.js", "patches/ds-bot/files/test/room-floor-once.test.js" ], "checks": { "ran": "TZ=UTC npm run verify (DSH_HARNESS set, PATH without pnpm); ARCH_DOCS_BASE=bd583bd0 arch-docs; ds-bot-patches generate/check/canonical; changelog check", "result": "green", "at": "613cca30b83174571d7237786920f2971e403a07" }, "next": "waiting for the review side's Verdict naming 613cca30b83174571d7237786920f2971e403a07" }