Skip to content

perf(desktop): stop idle and streaming render work - #5608

Merged
likun666661 merged 2 commits into
apache:mainfrom
Astro-Han:perf/desktop-streaming-render-energy
Sep 23, 2026
Merged

likun666661 merged 2 commits into
apache:mainfrom
Astro-Han:perf/desktop-streaming-render-energy

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

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 HoverCard rewrote anchor-name and 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 head dc0a598 only adds story assertions on top of 34959cd. Values are medians.

Metric Before After
Inline style writes / s while streaming 2099 404 / 387
… of which in the prompt rail 1255 0 / 0
Renderer task ms / s while streaming 930 862 / 862
Script ms / s while streaming 318 282 / 284
Style recalcs / s idle after streaming 17.9 0 / 0
Renderer task ms / s idle after streaming 32.5 21.2 / 23.1
Infinite animations running while streaming 2 1 / 1 (status dot, unchanged)
Renderer task ms / s, idle BrowserLoaded story 7.8 0.3 / 0.3

The 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: the scroll-window.mjs assertion "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 unpatched useTouchTrigger); 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).
  • BrowserLoaded story (session-workbar, also run by accessibility-runtime-surfaces), at normal speed and under 6× CPU throttling. Each step fails when the part of the watch that handles it is removed:
    • idle strip: no getBoundingClientRect over 250 ms (15 calls on main);
    • strip shifts, then shrinks in place: mirrored again;
    • an open popover moves, then grows, across the strip with no toggle: native view parked;
    • popover shown, then hidden or unmounted while open: parked, then restored;
    • an open, empty popover fills with content, like the toast viewport: parked, then restored.
  • Window resize shifting a right-anchored strip is checked in a Chromium harness only, since a story cannot resize its own window.
  • With the WorkHub docked in Electron, 0 requestAnimationFrame calls over 2 s at rest.
  • Desktop typecheck (all tsconfigs), npm run format, npm run lint clean. workhub-layout and session-workbar E2E passed locally at 34959cd; CI runs the full suite.
  • The regenerated @astryxdesign+core+0.6.2.patch applies to a pristine npm pack of 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 (in native-surface-occlusion.ts, next to the existing occlusion check) replaces the per-frame loop in the browser panel and WorkHub dock. It runs their existing sync in one coalesced frame when the element's box or an overlay above it may have changed:

  • ResizeObserver and window resize for size changes and window-driven shifts.
  • An IntersectionObserver whose root is shrunk to the element's own rect, threshold 1, for moves without resize.
  • toggle captured on the document, plus a MutationObserver on open but empty overlays, since astryx's toast viewport stays open and fills with no toggle.
  • Per-frame sampling only while a non-empty overlay is open (it can move or grow with no event), or while an ancestor clips the element (the move observer cannot see it move).

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:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code, for CDP profiling, diagnosis, implementation, and tests.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 23, 2026
@Astro-Han
Astro-Han force-pushed the perf/desktop-streaming-render-energy branch 2 times, most recently from 756ffd7 to a037a88 Compare September 23, 2026 04:24
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 23, 2026
@Astro-Han
Astro-Han force-pushed the perf/desktop-streaming-render-energy branch from a037a88 to a8e7214 Compare September 23, 2026 04:40
@Astro-Han
Astro-Han marked this pull request as ready for review September 23, 2026 05:07

@Sun-GLiang Sun-GLiang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 orangeCatDeveloper left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found two issues that should be addressed before approval:

  1. P2 — watchNativeSurface misses geometry changes to an already-open overlay. The observer at native-surface-occlusion.ts:50-58 watches 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 neither sync nor 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.

  2. P2 — the idle benchmark starts before the Storybook play function is known to be finished. scripts/perf/storybook.mjs:61-86 resolves __storyDone on storyRendered; the repository's smoke runner correctly waits for storyFinished. The fixed one-second delay can therefore overlap the BrowserLoaded.play interactions, and any later playFunctionThrewException is 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 successful storyFinished event 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.

@likun666661

Copy link
Copy Markdown
Member

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 storyFinished before measuring idle time. A regression for the first two paths and a trustworthy benchmark gate would establish that the removed polling/render work is not merely traded for missed updates or unmeasured work. Then keep each observer or fallback only where an actual change source requires it. I am not suggesting that #5606 and #5608 must be coupled for merge.

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
@Astro-Han
Astro-Han force-pushed the perf/desktop-streaming-render-energy branch 2 times, most recently from 06e4054 to 34959cd Compare September 23, 2026 06:07
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
@Astro-Han
Astro-Han force-pushed the perf/desktop-streaming-render-energy branch from 34959cd to dc0a598 Compare September 23, 2026 06:16
@Astro-Han

Copy link
Copy Markdown
Contributor Author

Thanks. The three points are addressed at dc0a598:

  • WorkHub highlight: PromptAnchorRail hands ticks one stable callback that calls the latest onHighlightTurn. A ChatView regression passes a fresh handler on every render, as WorkHub does, and checks that a streaming delta re-renders only the streaming turn's tick. It fails without the fix.
  • Open overlay geometry: while a non-empty overlay is open, the watch samples every frame until it closes. The BrowserLoaded story moves, then grows, an open popover across the strip with no toggle, and expects the native view parked each time.
  • The idle probe waits for a successful storyFinished and fails on the exception events.

On necessity, I removed each part of watchNativeSurface in turn and re-ran the BrowserLoaded story at normal speed and under 6× CPU throttling:

Removed Change it then misses Caught by
ResizeObserver strip shrinks in place (the move observer stays at ratio 1) story, shrink step
window resize window widens and a right-anchored strip shifts along with the observer root Chromium harness only; a story cannot resize its own window
move IntersectionObserver strip shifts without resizing story, shift step
toggle capture popover opens over the strip story, popover steps
MutationObserver on open, empty overlays toast viewport (astryx opens it on mount) fills with no toggle story, toast step
frame sampling while an overlay is open open popover moves or grows story, move/grow steps
frame sampling while the strip is clipped a strip clipped by an ancestor moves back; the move observer never reaches threshold 1 story, shift step (this also caught a real bug in CI on the previous head)

With no overlay open and the strip fully visible, nothing runs: 0 requestAnimationFrame calls over 2 s with the WorkHub docked in Electron, and 0.17 ms/s renderer task time in the idle story.

中文版

谢谢。三点都已在 dc0a598 处理:

  • WorkHub 高亮:PromptAnchorRail 给各刻度传一个稳定的回调,内部调用最新的 onHighlightTurn。新增的 ChatView 回归测试像 WorkHub 一样每次渲染都传入新的 handler,检查一次流式增量只重渲染正在流式输出的那个刻度;去掉修复会失败。
  • 已打开 overlay 的几何变化:只要有非空 overlay 打开,watch 就逐帧采样直到它关闭。BrowserLoaded story 在不触发 toggle 的情况下,先移动、再放大一个已打开的 popover 使其越过条带,每次都要求原生视图被停放。
  • 空闲探针等到成功的 storyFinished 才开始测量,遇到异常事件直接失败。

关于必要性:我逐个移除 watchNativeSurface 的每一部分,在正常速度和 6 倍 CPU 降速下重跑 BrowserLoaded story:

移除 随之漏掉的变化 由谁发现
ResizeObserver 条带原地缩小(移动观察器的 ratio 始终为 1) story 缩小步骤
window resize 窗口变宽,右锚定的条带随观察器 root 一起平移 只有 Chromium harness;story 无法改变自身窗口大小
移动检测 IntersectionObserver 条带平移但尺寸不变 story 平移步骤
toggle 捕获 popover 在条带上方打开 story popover 步骤
空的已打开 overlay 上的 MutationObserver toast viewport(astryx 挂载即打开)被填入内容,没有 toggle story toast 步骤
overlay 打开期间的逐帧采样 已打开的 popover 移动或变大 story 移动/放大步骤
条带被裁剪期间的逐帧采样 被祖先裁剪的条带移回原位,移动观察器永远达不到阈值 1 story 平移步骤(在上一个 head 的 CI 上也因此抓到了一个真实 bug)

没有 overlay 打开且条带完整可见时不做任何工作:Electron 中 WorkHub 停靠时 2 秒内 requestAnimationFrame 调用为 0,空闲 story 的 renderer task 时间为 0.17 ms/s。

@Astro-Han

Copy link
Copy Markdown
Contributor Author

Addressed at dc0a598.

@Sun-GLiang (automated review, P2 WorkHub ticks): PromptAnchorRail now keeps the latest onHighlightTurn in a ref and hands ticks one stable callback, so a fresh handler from ChatView no longer defeats tick memoization. prompt-rail-reading-position.test.tsx renders ChatView with a fresh onPromptRailHighlight on every render, as WorkHubConversation does, and asserts a streaming delta re-renders only the streaming turn's tick; it fails without the fix.

@orangeCatDeveloper (automated review):

  1. Open overlay geometry: while a non-empty overlay is open, the watch samples every frame until it closes. The BrowserLoaded story moves, then grows, an open popover across the strip with no toggle and expects the native view parked each time; it fails on the previous head. Removing each part of the watch in turn showed one more gap, now fixed and covered: a strip clipped by an ancestor could move without the move observer firing.
  2. storyFinished: the idle probe now waits for a successful storyFinished and rejects on status: 'error' and the exception events.
  3. The body now has a short Summary, with the headline before/after at the start of Verification.
  4. Preload timeout: the mark now runs four finite cycles (9.6 s), which outlast the overlay on both the content-ready and the 8 s timeout paths.

@likun666661
likun666661 merged commit 54135dc into apache:main Sep 23, 2026
2 checks passed
@Astro-Han
Astro-Han deleted the perf/desktop-streaming-render-energy branch September 23, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants