Fix three renderer perf metrics that lied, and the bug they hid - #290
Merged
Conversation
React's production build compiles out enableProfilerTimer, so <Profiler>'s
onRender never fires and rendererPerf's reactCommits reads 0 in every release
build. react-dom-client.production.js contains zero occurrences of "onRender";
the profiling build has three.
This was not theoretical. Across perf.log and perf.log.1, "reactCommits":0
appears in 100% of ~1,823 renderer samples and never once nonzero, so every
`react=0c/0ms` in the trace is a measurement artifact rather than evidence that
React is idle. Two separate perf investigations read that as "React is fine"
and went looking at the main process.
Verified by controlled experiment rather than inspection, since instrumentation
that looks correct while reporting zero is the whole bug. Identical source, two
production builds: without the alias reactCommits is 0, with it 6.
Only the client entry is aliased. react-dom-profiling.profiling.js itself does
require("react-dom") for ReactDOMSharedInternals, so aliasing the bare specifier
creates a cycle that leaves the internals undefined and kills the app at startup
on `reading 'd'` — confirmed by hitting it. The mixed import graph the renderer
actually uses (bare-react-dom createPortal, a second createRoot for view zones)
was exercised against the fix and is clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`performance.memory` is quantized and Chrome serves a cached value for ~20 minutes on pages that aren't cross-origin isolated, so the renderer cannot usefully measure its own memory. In the wild this meant heapUsedMB sat at exactly 560.8 for 40 minutes across 617 samples — 9 distinct values in an entire log — while the renderer's real RSS swung 600MB inside 30 seconds. heapGrowthMB/heapReclaimedMB are computed from that value, so they could not show an allocate-and-collect sawtooth, which is the precise shape they were added to catch. Worse, when the cached value does refresh, a 20-minute delta gets attributed to a single 1-second bucket. `[snapshot]` now carries rendererRssMB and rendererCpuPct sampled in main via app.getAppMetrics(), which is unquantized and costs the renderer nothing. It's scoped to BrowserWindow webContents — browser tabs are WebContentsViews in their own renderer processes, and folding those in would make the app renderer look like it ballooned whenever a heavy page was opened. Provider is injected rather than imported so the headless build doesn't pull in electron; there it stays null instead of reporting a number it can't measure. The performance.memory-derived fields keep their values but are suffixed …Quantized so a future reader can't mistake them for live, same reasoning as the earlier mainRss/rendererHeap prefixing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rendererBlockingMsPerSec logged the renderer bucket's raw blockingMs. That bucket is only nominally 1s — it stretches whenever the renderer's timer is starved, and a DevTools heap snapshot produced a single ~104s bucket that surfaced as blocked=103792ms/s. Read as a rate, those totals overstate blocking by 20-100x and make a mostly-idle renderer look pegged, which is exactly the misreading that sent this investigation chasing a stall that wasn't there. Normalize by elapsedMs and log rendererBucketMs alongside, so the window is visible and the raw totals stay recoverable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The react-dom/profiling alias added in this branch fixed reactCommits reading a permanent 0, but React 19.2's profiling entry emits a performance.measure() per component render for the DevTools Performance track. In a trace of a loaded session that logging was ~15% of total renderer CPU, plus 21k retained PerformanceMeasure objects in a heap snapshot taken while nothing was recording — a permanent tax on every user for a counter that only matters while someone is debugging. Gate it behind HARNESS_REACT_PROFILING=1. Samples now carry reactProfiling so perf.log and the HUD render "n/a" instead of the 0 that sent two prior investigations to the wrong process, and computeFlags can't raise a react flag off numbers nobody measured. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The renderer's dominant commit-phase cost was traversing a fiber tree that never shrinks. App keeps every worktree mounted, and the whole `statuses` / `shellActivity` maps were threaded from App down through WorkspaceView → SplitRenderer → LeafPane → TerminalPanel. A status dispatch fires per streamed token, so with ~22 worktrees mounted each token allocated a new map and re-rendered all N subtrees. Three changes, in dependency order: 1. Narrow the reads. `useTerminalStatus(id)` / `useShellActivity(id)` return per-terminal values, so the maps stop being props anywhere in that chain. Both selectors return primitives/stable refs, so Object.is dedupes and only the tab that actually changed re-renders. 2. Stabilize the callbacks App passes down — an inline arrow allocates N new closures per App render and would defeat the memo before it compares anything else. 3. Memoize WorkspaceView. Only sound after (1); while the maps were props their identity changed every token and this would have been a no-op. Profile that motivated it (36s trace, renderer pid): updateProperties 5.3%, commitRoot 3.9%, commitHostUpdate 1.4%, with a hot stack of 20+ nested commitMutationEffectsOnFiber ↔ recursivelyTraverseMutationEffects. Heap snapshot showed 254,687 live FiberNodes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch started as "two telemetry fields report constants" and ended up finding the renderer bug those constants were hiding. Four changes, in the order they happened.
1.
reactCommits/reactTotalMswere structurally0in every packaged build. React's production build compiles outenableProfilerTimer, so<Profiler>'sonRenderis never called —react-dom-client.production.jscontains zero occurrences ofonRender."reactCommits":0appeared in 100% of ~1,823 samples across two log files and was never once nonzero.2.
heapUsedMBand its deltas are quantized and up to ~20 minutes stale. Chrome cachesperformance.memoryon pages that aren't cross-origin isolated. Observed pinned at exactly560.8for 40 minutes across 617 samples (9 distinct values in an entire log) while real RSS swung 600MB inside 30 seconds — soheapGrowthMB/heapReclaimedMBcannot show the allocate-and-collect sawtooth they were added to catch. Replaced in[snapshot]with real per-processrendererRssMB/rendererCpuPctviaapp.getAppMetrics(), scoped toBrowserWindowwebContents. Theperformance.memory-derived fields are kept but suffixed…Quantized.3.
rendererBlockingMsPerSecwas not per second. It logged the bucket's rawblockingMs. That bucket is nominally 1 s but stretches without bound when the renderer's timer is starved — a DevTools heap snapshot produced a single ~104 s bucket, surfacing asblocked=103792ms/s. Reading those totals as rates overstates blocking by 20-100x. Now normalized byelapsedMs, withrendererBucketMsandrendererBlockingMsTotallogged alongside so the raw numbers stay recoverable.4. One terminal's status change re-rendered every mounted worktree. This is the bug the constants were hiding, and the reason the app got progressively laggier over a session.
Appkeeps every worktree mounted. The wholestatuses/shellActivitymaps were threaded fromAppdown throughWorkspaceView→SplitRenderer→LeafPane→TerminalPanel. A status event fires per streamed token, and each one allocated a new map — so with ~22 worktrees open, every token re-rendered all N subtrees even though 21 of the 22 statuses were unchanged.Fixed in three dependent steps:
useTerminalStatus(id)/useShellActivity(id)mean the maps are no longer props anywhere in that chain. Both return a primitive or a stable ref, souseSyncExternalStore'sObject.ischeck dedupes and only the tab that actually changed re-renders.Apppasses down — inline arrows allocated N new closures perApprender.WorkspaceView. Only sound after the first step; while the maps were props their identity changed every token and this would have been a no-op.The profiling alias, and why it is now opt-in
Change 1 originally landed as a permanent
react-dom/client→react-dom/profilingalias. That was reverted to opt-in (HARNESS_REACT_PROFILING=1) in a5ca2c9, and the reversal is the most transferable thing on this branch.React 19.2's profiling entry emits a
performance.measure()per component render to populate the DevTools Performance track. In a 36 s trace of a loaded session,logComponentRenderwas 10.8% of total renderer CPU — the largest named entry after idle and(program)— withlogComponentEffectand theperformance.now()calls feeding them on top, plus 21k retainedPerformanceMeasureobjects in a heap snapshot taken while nothing was recording.So: instrumentation added to explain a slowdown became a measurable share of it. Default builds now ship
reactProfiling: falseon every sample, and every consumer rendersn/a— never0, which is what caused the original misreading.Honest note on measurement
The lag accumulates over hours, so there is no synthetic repro and no clean A/B. What we have is a maintainer running the merged build for a couple of days and reporting a large improvement. That is a real signal but it is not a number, and the baseline it is measured against included our own profiling overhead from change 1's first form. Some of the improvement is this branch getting out of its own way rather than change 4 working. Both are worth having; they should not be conflated.
Notes for review
electron.vite.config.tscarries a long comment for a reason — do not add a barereact-domalias.react-dom-profiling.profiling.jsitself doesrequire("react-dom")to reachReactDOMSharedInternals, so aliasing the bare specifier points that lookup back at the profiling build; the cycle leaves the internals undefined and the app dies at startup onreading 'd'. The alias is also an anchored regex rather than a plain string, because aliasfindmatches on a/prefix and'react-dom'would additionally rewritereact-dom/server.CLAUDE.md is updated with all three telemetry defects, the anti-pattern behind change 4, and the general rule: a telemetry field that reads a constant is not evidence of a quiet system, it is evidence of broken instrumentation. Including the cheap check that would have caught two of these in seconds:
Test plan
npm run typecheckcleannpx electron-vite buildcleannpx vitest run src/renderer src/shared— 1504 tests passcommits:0, aliased reportscommits:6react-domcreatePortal+ a secondcreateRoot) returns{"commits":6,"buildIsProfiling":true,"portalOk":true,"secondRootOk":true}— confirms the cycle hazard above is avoidedlogComponentRender;HARNESS_REACT_PROFILING=1build contains 5🤖 Generated with Claude Code