Repository navigation
perf(messages): keep recent markdown parses across row remounts - #1967
Conversation
switching channels remounts every message row, so revisiting a channel prepared and parsed every unchanged body again. keep recent prepareMarkdown results and react-markdown element trees keyed by the protected content. weigh parses by element count and key length, and skip any result heavier than its budget, so each cache stays near 4 mb even for dense markup or long bodies. the parsed elements depend only on that content; their components still read the rendering row's context. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
df71e8e to
812c123
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blockers at 812c123. 9/10. The parse cache key covers every ProtectedContent field, and the other parser inputs are module constants. Row, session and channel behavior stays behind the context-reading component types, so cached trees don't carry bindings between rows. LRU accounting is consistent across hits, eviction and oversized results. With react-markdown 10.1.0 pinned, the default export is synchronous and hook-free (runSync), and an async plugin still throws as it did before.
A production-React Node probe agreed with the budget estimate: a 99k-character paragraph retained about 155 KB, roughly 3 MB at the full budget. That is a plausible estimate, not a heap ceiling. Prepared results and live trees are on top of it.
The two inline comments are optional. Hosted checks are green, and no suites were rerun locally.
| protectedContent.prefix, | ||
| protectedContent.parts, | ||
| ]), | ||
| // The default export is a hook-free function: calling it keeps its result. |
There was a problem hiding this comment.
🤖 Optional: tie this to the pinned react-markdown version (10.1.0). A cache miss calls Markdown and a hit doesn't. If a future default export used hooks, hook order would differ between those renders, and nothing here would warn an upgrader.
| expect(screen.getByText(":party_parrot:")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("reuses a remounted body's parse while each row reads its own inputs", () => { |
There was a problem hiding this comment.
🤖 Optional: nothing observes the preparedBodies cache. These tests would still pass with that wrapper removed. Counting prepareMarkdown calls in this remount test would cover it without a new test.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blocking findings at 812c123. The cache key covers everything the parse reads: content, prefix and parts, plus module-constant plugins, transformUrl and markdownComponents. Per-row inputs still come from MessageComponentsContext at render time.
What we checked, beyond hosted CI (green):
- react-markdown 10.1.0's sync export is hook-free.
- StrictMode probes with one shared cached tree: each mount keeps its own state, provider updates arrive through bailouts, and a remount gets fresh state, all with one parse.
- A parse that throws reaches the error boundary and leaves nothing cached.
MessageMarkdown.test.tsxpasses in shuffled order (seeds 1–5), and each new test passes alone.
Validation gaps (not blockers):
- The ~4 MB budget is a production heuristic and no test measures retained heap.
- In development builds, cached elements keep
_owner/_debugStack, so dev heap can exceed it.
Two optional test-coverage notes are inline.
| ...actual, | ||
| // Hook-free, so the returned elements are the parse a caller keeps. | ||
| default: (properties: ComponentProps<typeof actual.default>) => { | ||
| markdownRenders(); |
There was a problem hiding this comment.
Optional: markdownRenders now counts cache misses, not MarkdownBody renders. That means the existing "without reparsing" assertions (:338, :350) no longer catch a memo regression.
We checked this by removing memo from MarkdownBody and adding directory to the protectedContent deps. Base fails (called 2 times, expected 1); this head passes. The cost would only be performance: protectInlineContent plus a JSON.stringify on every directory or profile update, and React still bails out on the same element.
To keep that coverage, count MarkdownBody renders separately from parses. Otherwise, accepting the loss is fine.
| expect(screen.getByText(":party_parrot:")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("reuses a remounted body's parse while each row reads its own inputs", () => { |
There was a problem hiding this comment.
Optional: this test varies only context inputs (channel names), not inputs that come from the key. If parts is dropped from the parsedBodies key, only an unrelated test fails (the agent icon at :892). The result would be two rows with the same text but different bound pubkeys sharing a tree, so one row's mention would open the other's profile.
This is an identity-binding invariant, so a direct guard is worth having. In this same test, render one body as two rows bound to different pubkeys under the same display name, and assert each one opens its own data-profile-target.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blockers at 812c123. Sharing parsed elements across rows is sound. The cache key (content, prefix, parts) covers every input the remark/rehype pipeline reads. transformUrl and markdownComponents are module constants, and per-row behavior (links, mentions, spoiler state, code renderers) still comes from MessageComponentsContext at render time. react-markdown 10.1.0's sync Markdown export is hook-free (MarkdownHooks is the hooked one), so calling it directly is safe at this version.
Local checks: MessageMarkdown.test.tsx passes (109/109), including three shuffled runs (seeds 1 to 3), so the module-level cache doesn't make the tests order-dependent. CI is green.
One optional note is inline.
| render("**evicted**"); | ||
| const words = "word ".repeat(19_000); | ||
| for (let index = 0; index < 25; index++) render(`${index} ${words}`); | ||
| const newer = markdownRenders.mock.calls.length; |
There was a problem hiding this comment.
Optional, test cost: locally this test takes ~1.4 s and the heavy-body test ~0.4 s, against ~2.8 s for the rest of the file. The cost comes with the approach. Overflowing a 4,000-unit budget by length alone takes about 2M characters of parsing, and MAX_MARKDOWN_LENGTH caps each body at 100k, so you need at least 21 near-cap parses. As the PR notes, this one also passes on main, so it guards a tuning constant rather than behavior. Fine to keep. If file time matters, you could check the weight function directly in Node instead.
Switching back to a channel no longer prepares and parses every unchanged message body again.
Only one channel reader is mounted at a time, so each switch remounts every visible row.
MessageMarkdownmemoizedprepareMarkdownand the react-markdown parse per mount, so it redid both for content the app had already rendered. It now keeps recent results in two module-level maps keyed by content: prepared bodies byrow.content, and parsed element trees by the protected content.Each map has a weight budget, and a result heavier than the whole budget is not kept. Measured with production React, a parsed body keeps about 2 KB, plus about 1 KB per element and about 2 bytes per character. Parses are therefore weighed by element count and key length, with a budget of about 4 MB. That holds for dense markup (10k characters of
*a*is 2.5k elements and 2.7 MB) and for long plain paragraphs (99k characters is 155 KB). Prepared bodies keep only their content and literal ranges, so they are weighed by length, up to 200k characters.The parsed elements depend only on the protected content. The components they render still read the rendering row's context, so links, mentions, channel names and spoilers come from the row that mounts them. react-markdown's default export is a hook-free function, so calling it directly keeps its result without changing what renders.
Chromium CPU profile over 12 switches at 100 channels:
Markdown69.5 → 3.6 ms, andprepareMarkdownis no longer sampled.Blox workstations, main 561265d against this commit: 5 Linux and 5 macOS, production builds, Playwright WebKit 26.6 and Chromium 153. The fixture has 100 channels with 10 thread tabs each, 200 history rows, 100 thread replies and background traffic. Each machine ran every cell once per variant, interleaved with this change stacked on #1966.
Switch latency over 5 runs. Switching between two channels gives 55 samples. Switching around 10 open channels in turn gives 95 samples, and each body has to survive nine other channels in the cache:
Linux WebKit (WebKitGTK) two-channel switching varies from run to run. Per-machine medians for this commit range from 405 to 450 ms. Two earlier rounds of the same parse cache measured −8% (434 → 400) and −7% (435 → 405) there.
Medians of 5 runs over a 20 s idle window with background traffic. This change is not expected to move them:
Earlier rounds of the same parse cache, whose weights differed but measured the same, covered three more cases:
Validation:
MessageMarkdown.test.tsxhas four new tests:Three fail on main. The length test passes on main, which parses every time, and fails with the length weight removed. The heavy-body test also fails with the guard removed.
pnpm check, and Vitest forsrc/features/messages(2212/2212).messages,mentions,composer-links,channel-tabs,terminal-renderer,navigation-thread-history(101 passed; 1 skipped by design in Chromium).🤖 Generated with Claude Code