Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I ran the reproduction from #54305 on the base and with this PR, and the PR doesn't improve per-update cost. With one paragraph growing by one word per update, it measured 13.7 ms on the base and 14.9 ms with this PR at 3000 words (3.7 vs 4.0 ms at 1000). When I split the text into many short paragraphs, the case this change targets, both came out at about 10.6 ms. The existing tests in markdown-solid.test.tsx and the session-ui type check pass, and the rendered output looks unchanged.
I timed each step of update with the PR at 3000 words:
reuseNodestakes about 5 ms per update.reconciletakes about 7 ms when one word changes, and about 3 ms even when the HTML is identical.
So the cost has moved into the comparison rather than gone away. The changed paragraph still gets fully reconciled, and that paragraph is exactly the part that changes on every update. This matches your own result that the end-to-end gain sits inside the noise. Details are in the inline comments.
The public API is unchanged (internal to the session-ui markdown renderer).
As it stands, I don't think this resolves #54305. Its expected result is a near-constant cost per update when one word is appended. If you rework it, a small test in markdown-solid.test.tsx would help: stream words, check the DOM matches a fresh render, and check that existing word spans keep their elements.
| // A delta only changes the tail of the block, so most of the parsed tree is identical to what | ||
| // the store already holds. Handing `reconcile` the store's own nodes for those regions keeps it | ||
| // (and `unwrap`) from walking the whole array on every delta. | ||
| setNodes(reconcile(untrack(() => reuseNodes(nodes, parsed)), { key: "key" })) |
There was a problem hiding this comment.
reuseNodes(nodes, parsed) reads the whole old tree through the store proxy (every property read goes through the proxy's get trap, even under untrack). In the #54305 benchmark it costs about 5 ms per update at 3000 words, roughly what it saves in unwrap. unwrap(nodes) made no measurable difference. Per-update cost was 13.7 ms on the base and 14.9 ms with this PR. To avoid the O(n) work you would need to keep the previous plain parsed array and update only the changed node and the new children by path (setNodes(...path)), instead of passing a full tree to reconcile.
| if (sameNode(previous, node)) return previous | ||
| if (previous.type !== "element" || node.type !== "element") return node | ||
|
|
||
| return { ...node, children: reuseNodes(previous.children, node.children) } |
There was a problem hiding this comment.
The node that changes on every update (the live paragraph or list item) is rebuilt here as a new object, so reconcile still runs its keyed diff over all of that node's children. In one long paragraph that is every word, and it still took about 7 ms per update at 3000 words. Reusing the unchanged siblings doesn't reduce this part, which is where most of the time goes in the benchmark.
sameNode compares a whole subtree before reuseNode either reuses it or recurses into it. A matching subtree, like an unchanged block, is walked down to every word on each update. So the comparison is still linear in the size of the block. Even with identical HTML it took about 4.8 ms per update at 3000 words.
|
Reworked along the direction in your inline comments. You were right on both counts: The renderer now keeps the plain Per-update cost for one paragraph growing by a word, measured the way you did (mean of the last 100 updates, production build, chromium):
The store walk alone (no store write, no DOM) is 0.05ms at 1000 words and 0.18ms at 3000 — near-constant. What remains is the parse (~0.9ms) and One thing worth noting: wrapping the writes in The test you suggested is in Behaviour was checked against the base across 16 scenarios (long words, |
Te-River
changed the title
|
One note on overlap, since #54305's review mentioned #48432: that one is in |
34aa3ca to
0848913
Compare
0848913 to
85ad112
Compare
|
Complementary to #55857 (no overlap) Opened #55857, which ports #48432's incremental live-tail projection to V2 in |
85ad112 to
0d46485
Compare
9c12efb to
6b7fd4e
Compare
There was a problem hiding this comment.
Both points from my last review are addressed. update now walks the plain tree and writes only the tail by path, and the live node is extended in place instead of being rebuilt.
I reran the benchmark from #54305 on the base and with this PR:
- At 3000 words, per-update cost went from about 34 ms to about 17–18 ms; at 1000 words, from about 9 ms to about 3 ms.
- The tree walk itself is about 0.7 ms per update at 3000 words. Most of what remains is the store write plus
For's DOM update, which the base pays too. updatefell back toreconcileonly once in 100 updates, on the first update whenanimatechanges.- Existing word spans keep their elements, and only new words get
data-markdown-enter. - The final DOM matches a fresh render for a word growing in place, toggling bold, inline code, lists, shrinking text and new blocks.
bun run test (253 pass) and bun run typecheck in packages/session-ui pass.
The public API is unchanged.
One new concern, on the test (see the inline comment): it relies on a hand-written React.createElement shim set on globalThis.
packages/session-ui/src/components/markdown-solid.test.tsx:6: Without this shim the new test errors, so it runs the renderer through a hand-written JSX runtime rather than Solid's compiled output. Under the shim, props and children are read once instead of tracked. So{node.text}in a word span isn't reactive, and the in-place text update for a grown word (set(path.concat("text"), …)) isn't really checked. The test also setsglobalThis.Reactfor every test file in the samebun testprocess (the package script has no--isolate). I'd move the streaming check to the Playwright component tests, which compile with Solid. Failing that, check only the node-level behaviour and drop the shim, rather than keep a test that runs a different renderer from production.
6b7fd4e to
20d5354
Compare
There was a problem hiding this comment.
The code is the same as the version I reviewed last time, and the rebase keeps it working. I reran the benchmark from #54305: at 3000 words, per-update cost is about 32.7 ms on the base and 12.7 ms with this PR (6.4 vs 2.9 ms at 1000 words). The final DOM still matches a fresh render across the streaming cases I tried. bun run test (253 pass) and tsc -b in packages/session-ui pass.
The public API is unchanged.
One thing to fix: resolving the rebase in packages/session-ui/package.json undid two changes from dev (see the inline comment).
packages/session-ui/package.json:3: This looks like a leftover from the rebase. Compared withdev, the PR now changesversionback from2.1.4to2.1.2. It also removes"typecheck:tsc": "tsc -b"a few lines down. CI's typecheck job runsbun run typecheck:tscacross packages through turbo, so session-ui would quietly stop being type-checked withtsc. Could you restore both fromdev, so the only change here is--conditions=browserontest?
20d5354 to
356a3dc
Compare
There was a problem hiding this comment.
The rebase leftover in packages/session-ui/package.json is fixed: version and typecheck:tsc match dev again, and the only change there is --conditions=browser on test. The renderer code is unchanged since my last review, so the earlier benchmark result still applies (about 32.7 ms → 12.7 ms per update at 3000 words). bun run test (253 pass) and bun run typecheck:tsc in packages/session-ui pass.
The public API is unchanged.
No further concerns.
356a3dc to
7699d86
Compare
There was a problem hiding this comment.
This push is a rebase only. The PR's changes to packages/session-ui are identical to the head I reviewed last time. On the rebased tree, bun run test (253 pass) and bun run typecheck:tsc in packages/session-ui pass. The earlier benchmark result still applies (about 32.7 ms → 12.7 ms per update at 3000 words).
The public API is unchanged.
No further concerns.
7699d86 to
2133807
Compare
There was a problem hiding this comment.
This push is a rebase only. The PR's changes to packages/session-ui are identical to the head I reviewed last time. On the rebased tree, bun run test (253 pass) and bun run typecheck:tsc in packages/session-ui pass. The earlier benchmark result still applies (about 32.7 ms → 12.7 ms per update at 3000 words).
The public API is unchanged.
No further concerns.
Temporary: it is anomalyco#54328. Used here to measure the markdown store cost in anomalyco#54305.
A delta only changes the tail of a block, so most of the parsed tree is identical to what the store already holds. Handing reconcile the store's own nodes for those regions keeps it, and the unwrap it runs first, from walking the whole array on every delta. A renderer profile of 600 visible deltas puts the store walk at 536ms of 2880ms sampled; this halves it (unwrap 536 -> 279, applyState 78 -> under 22). The end-to-end elapsed is dominated by the block's innerHTML re-parse, so the visible gain is small.
The previous attempt handed reconcile a tree rebuilt from the store's own nodes. That moved the cost into the comparison: reuseNodes read the whole old tree through the store proxy (~5ms per update at 3000 words), and the live node was still rebuilt, so reconcile diffed all of its children (~7ms). Keep the plain parsed tree instead, walk the tail by path, and write only the changed node and the new children with setNodes(...path). The live node is extended in place, so a grown word keeps its span. A delta that changes more than the tail falls back to reconcile. Per-update cost for one paragraph growing by a word: 16.8ms base, 13.1ms with the previous attempt, 3.4ms here at 3000 words. The store walk is 0.18ms and near-constant; what remains is the parse and For's DOM diff, which the base pays too.
2133807 to
d930ef7
Compare
There was a problem hiding this comment.
This push is a rebase only. The PR's changes to packages/session-ui are identical to the head I reviewed last time. On the rebased tree, bun run test (253 pass) and bun run typecheck:tsc in packages/session-ui pass. The earlier benchmark result still applies (about 32.7 ms → 12.7 ms per update at 3000 words).
The public API is unchanged.
No further concerns.
|
Retiring this to stay under the repository's open pull request limit; the branch is unchanged and the finding is recorded in #54305. |
Issue for this PR
Closes #54305
Type of change
What does this PR do?
A streaming delta only appends to the tail of a block, so the store can be updated by path instead of handing the whole tree to
reconcile.The renderer keeps the plain
parsedtree, walks the tail by path, and writes only the changed node and the new children withsetNodes(...path). The live node is extended in place, so a grown word keeps its span. A delta that changes more than the tail (shorter tree, changed prefix, word/element swap) falls back toreconcile.How did you verify your code works?
Per-update cost for one paragraph growing by one word, mean of the last 100 updates, production build, chromium:
The store walk alone (no store write, no DOM) is 0.05ms at 1000 words and 0.18ms at 3000 — near-constant. What remains is the parse (~0.9ms) and
For's DOM diff (~2.3ms), which the base pays too.The first attempt handed
reconcilea tree rebuilt from the store's own nodes. That moved the cost into the comparison:reuseNodesread the whole old tree through the store proxy (~5ms per update at 3000 words), and the live node was still rebuilt, soreconcilediffed all of its children (~7ms). Wrapping the writes inbatch()also matters: without it, eachsetNodesre-diffs the block, and 3000 words goes from 3.4ms to 4.9ms.markdown-solid.test.tsxgains a streaming test: it appends a word at a time, asserts the existing spans keep their elements, and compares the result against a one-shot render.**toggling, word/element swaps, inline code, links, new blocks, lists, code fences, shrink, headings): final DOM, span retention anddata-markdown-entercounts all match.bun test --conditions=browser srcinpackages/session-ui: 250 passed, 0 failed. The test script gains--conditions=browserfor the same reasonpackages/apphastest:browser.The end-to-end elapsed in the #54305 benchmark is dominated by the block's
innerHTMLre-parse (#54428), so the visible gain there is smaller than the per-update numbers above.Screenshots / recordings
n/a
Checklist
Added by the opencode bot (AI-generated).
Related: #54428, #54295