perf(desktop): stop idle and streaming render work - #5608
Conversation
756ffd7 to
a037a88
Compare
a037a88 to
a8e7214
Compare
Sun-GLiang
left a comment
There was a problem hiding this comment.
Automated review — exact head a8e72148334c7e11cd6bfca44b68e5cf750a7442
P2 — WorkHub streaming still rerenders every prompt-rail tick. The new reusePromptAnchorRailTurns call in packages/ui/src/chat-view.tsx:464 preserves unchanged turn entries, but ChatView creates a new onHighlightTurn function on every render at :775 whenever highlighting is enabled. WorkHubConversation always enables it (apps/desktop/src/renderer/features/workhub/ui/workhub-conversation.tsx:137). That changing prop defeats PromptAnchorRail's memo; its hoverTurn callback changes as well, so every memoized PromptRailTick receives new onHover and onHighlight props and rerenders on each streaming delta. The 30-prompt performance probe exercises a normal Session, so its zero prompt-rail style writes do not establish the same result for WorkHub.
Please keep the highlight wrapper stable while calling the latest supplied handler, then add a WorkHub streaming regression that verifies unaffected ticks stay mounted without rerendering. This finding follows the render and callback path in the exact-head source; I did not run a WorkHub performance measurement. I found no other confirmed P0–P2 issue in this diff.
This is a comment on the code at this head, not an approval or merge action.
Automated review notice: This review was generated by Codex for Sun-GLiang. It is not an independent human review and does not replace one.
orangeCatDeveloper
left a comment
There was a problem hiding this comment.
I found two issues that should be addressed before approval:
-
P2 —
watchNativeSurfacemisses geometry changes to an already-open overlay. The observer atnative-surface-occlusion.ts:50-58watches only child-list mutations. After an overlay has opened and its finite animation has settled, changing its size or position through text, attributes, CSS anchor positioning, or inline styles triggers neithersyncnor a new occlusion check. If that change moves the overlay across a native surface, the native view remains visible above it. Please observe overlay geometry as well and add a regression where an open popover moves or grows across the strip without another toggle. -
P2 — the idle benchmark starts before the Storybook
playfunction is known to be finished.scripts/perf/storybook.mjs:61-86resolves__storyDoneonstoryRendered; the repository's smoke runner correctly waits forstoryFinished. The fixed one-second delay can therefore overlap theBrowserLoaded.playinteractions, and any laterplayFunctionThrewExceptionis ignored because the promise has already resolved. This also makes the baseline and candidate less comparable because this PR lengthens that play function. Please wait for the matching successfulstoryFinishedevent before settling and sampling.
Repository compliance: the PR body should also be tightened to the required 2-3 sentence ## Summary, with the headline improvement and minimal before/after evidence moved to the start of ## Verification.
Non-blocking: the eight-second preload timeout still hides the overlay without setting display: none, so the breathing animation continues indefinitely when no content-ready marker ever appears.
This is an automated review comment on exact head a8e72148334c7e11cd6bfca44b68e5cf750a7442, not an approval or a substitute for independent human review.
|
Looking at this alongside #5606, the common direction is sound: do work when the state that determines the result actually changes. #5606 moves Host readiness to the owning lifecycle and removes the renderer’s timed boundary retry; this PR aims to replace idle per-frame checks and unchanged streaming renders with the relevant change signals. For this PR, the Occam test is whether the new observation machinery is both necessary and complete. Before approval, please address the already-raised review points on (1) geometry changes to an open, settled overlay, (2) WorkHub’s unstable highlight callback defeating prompt-rail memoization, and (3) waiting for Storybook |
Adds a streaming-render Electron probe to the frontend performance lane and a browser-panel idle case to the Storybook driver. Both run without reduced motion, so infinite animations and per-frame geometry polling show up, and report style writes, running infinite animations, CDP style recalcs and renderer task time per second. Generated-by: Claude Code
06e4054 to
34959cd
Compare
Normal conversations kept the renderer and GPU busy with work that changed nothing on screen: - The browser panel and WorkHub dock polled their geometry every frame while a page was showing, even when idle (~120 style recalcs/s). They now use watchNativeSurface: ResizeObserver, window resize and an IntersectionObserver armed on the element's own rect for moves, plus toggle capture for overlays. Frames are sampled only while a non-empty overlay is open, since its geometry can change without any event, or while an ancestor clips the element, since the move observer cannot see a clipped element move. - The prompt rail replaced every entry on each streaming delta, and a caller's fresh highlight handler re-rendered every tick (WorkHub). It now reuses unchanged entries and hands ticks a stable handler. - astryx useTouchTrigger returned a new object per render, so every HoverCard re-render detached and re-attached its trigger ref, rewriting anchor-name and rebinding listeners. The patch memoizes it. - The preload mark breathed forever, hidden, after the overlay faded; it now runs four cycles, which outlast the 8s overlay timeout. Generated-by: Claude Code
34959cd to
dc0a598
Compare
|
Thanks. The three points are addressed at dc0a598:
On necessity, I removed each part of
With no overlay open and the strip fully visible, nothing runs: 0 中文版谢谢。三点都已在 dc0a598 处理:
关于必要性:我逐个移除
没有 overlay 打开且条带完整可见时不做任何工作:Electron 中 WorkHub 停靠时 2 秒内 |
|
Addressed at dc0a598. @Sun-GLiang (automated review, P2 WorkHub ticks): @orangeCatDeveloper (automated review):
|
Summary
A normal streaming conversation kept the desktop renderer busy with work that changed nothing on screen: the browser panel and WorkHub dock polled their geometry every frame, the prompt rail re-rendered every tick on each delta, astryx
HoverCardrewroteanchor-nameand rebound listeners on every re-render, and the hidden preload mark animated forever. This PR removes that work without any visible change, so streaming does about a fifth of the inline style writes and an idle window with a browser page open does almost no renderer work.Verification
Headline, from the Performance frontend workflow on the same runner CPU and image (AMD EPYC 7763, image 20260920.314.1). Baseline is
main+ the probe (ec83118, run); candidate is this PR (34959cd, run 1, run 2). The final headdc0a598only adds story assertions on top of34959cd. Values are medians.stylewrites / s while streamingBrowserLoadedstoryThe count metrics (style writes, rail writes, idle recalcs, animations) hold across all eight runs of both commits, which landed on four different runner CPU/image combinations (other baseline runs: 1, 2, 3; final-head runs: 1, 2). Timings are only compared within one CPU and image. Streaming style recalcs and layouts per second rose slightly (59 → 64 / 63 and 30 → 33 / 33); both run at most once per frame, and with task time down the likely reading is that more frames get drawn, but these counters cannot show that. No counter here measures GPU frames. The probes are described in
scripts/perf/CI.md.Every run, including unmodified
main(control), fails two existing steps: thescroll-window.mjsassertion "height corrections must not reverse an upward reader" and the Storybook 45-tools wait. The new probes run first and report in every run.Tests:
@maka/ui: 619/619 pass. New:prompt-anchor-rail.test.ts: a streaming delta hands back every unchanged entry.prompt-rail-reading-position.test.tsx: a tick re-render binds no new listeners (fails on unpatcheduseTouchTrigger); with a fresh highlight handler on every render, as WorkHub passes, a streaming delta re-renders only the streaming tick (fails without the stable callback).BrowserLoadedstory (session-workbar, also run byaccessibility-runtime-surfaces), at normal speed and under 6× CPU throttling. Each step fails when the part of the watch that handles it is removed:getBoundingClientRectover 250 ms (15 calls onmain);requestAnimationFramecalls over 2 s at rest.npm run format,npm run lintclean.workhub-layoutandsession-workbarE2E passed locally at34959cd; CI runs the full suite.@astryxdesign+core+0.6.2.patchapplies to a pristinenpm packof 0.6.2 and reproduces the patched tree.No screenshots: nothing visible changes. The preload mark now breathes four cycles (9.6 s), which outlasts the overlay on both its content-ready and 8 s timeout paths.
Design
watchNativeSurface(innative-surface-occlusion.ts, next to the existing occlusion check) replaces the per-frame loop in the browser panel and WorkHub dock. It runs their existingsyncin one coalesced frame when the element's box or an overlay above it may have changed:ResizeObserverand windowresizefor size changes and window-driven shifts.IntersectionObserverwhose root is shrunk to the element's own rect, threshold 1, for moves without resize.togglecaptured on the document, plus aMutationObserveron open but empty overlays, since astryx's toast viewport stays open and fills with no toggle.The WorkHub dock also refreshes the watch when the Workbar props it reports change. The prompt rail reuses entries that still read the same and hands ticks one stable highlight callback. The astryx change memoizes
useTouchTrigger's return in the existing patch; upstream: facebook/astryx#6472.The pulsing "running" status dots are a separate, larger GPU cost. They are left as is here because replacing them is a visible design change.
AI use
Select exactly one:
Tool(s) and scope: Claude Code, for CDP profiling, diagnosis, implementation, and tests.
Checklist
Does this PR entail a change in behavior?