From d5bba18b5d81f234657586865248c5b6849599cd Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 15:27:04 +0100 Subject: [PATCH] fix[react-devtools]: record timeline data only when supported (#31154) Stacked on https://github.com/facebook/react/pull/31132. See last commit. There are 2 issues: 1. We've been recording timeline events, even if Timeline Profiler was not supported by the Host. We've been doing this for React Native, for example, which would significantly regress perf of recording a profiling session, but we were not even using this data. 2. Currently, we are generating component stack for every state update event. This is extremely expensive, and we should not be doing this. We can't currently fix the second one, because we would still need to generate all these stacks, and this would still take quite a lot of time. As of right now, we can't generate a component stack lazily without relying on the fact that reference to the Fiber is not stale. With `enableOwnerStacks` we could populate component stacks in some collection, which would be cached at the Backend, and then returned only once Frontend asks for it. This approach also eliminates the need for keeping a reference to a Fiber. --- .../src/backend/agent.js | 55 ++++++++------- .../src/backend/fiber/renderer.js | 18 +++-- .../src/backend/profilingHooks.js | 67 ++++++++++++------- .../src/backend/types.js | 6 +- packages/react-devtools-shared/src/bridge.js | 8 ++- .../react-devtools-shared/src/constants.js | 2 + .../src/devtools/ProfilerStore.js | 5 +- .../views/Profiler/ReloadAndProfileButton.js | 7 +- packages/react-devtools-shared/src/hook.js | 1 + packages/react-devtools-shared/src/utils.js | 13 +++- 10 files changed, 123 insertions(+), 59 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 12704899ec..e05abf51ab 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -153,12 +153,17 @@ export default class Agent extends EventEmitter<{ _persistedSelection: PersistedSelection | null = null; _persistedSelectionMatch: PathMatch | null = null; _traceUpdatesEnabled: boolean = false; - _onReloadAndProfile: ((recordChangeDescriptions: boolean) => void) | void; + _onReloadAndProfile: + | ((recordChangeDescriptions: boolean, recordTimeline: boolean) => void) + | void; constructor( bridge: BackendBridge, isProfiling: boolean = false, - onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, + onReloadAndProfile?: ( + recordChangeDescriptions: boolean, + recordTimeline: boolean, + ) => void, ) { super(); @@ -658,17 +663,19 @@ export default class Agent extends EventEmitter<{ this._bridge.send('isReloadAndProfileSupportedByBackend', true); }; - reloadAndProfile: (recordChangeDescriptions: boolean) => void = - recordChangeDescriptions => { - if (typeof this._onReloadAndProfile === 'function') { - this._onReloadAndProfile(recordChangeDescriptions); - } + reloadAndProfile: ({ + recordChangeDescriptions: boolean, + recordTimeline: boolean, + }) => void = ({recordChangeDescriptions, recordTimeline}) => { + if (typeof this._onReloadAndProfile === 'function') { + this._onReloadAndProfile(recordChangeDescriptions, recordTimeline); + } - // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. - // In that case, the shell must also listen for this specific message to know when it needs to reload the app. - // The agent can't do this in a way that is renderer agnostic. - this._bridge.send('reloadAppForProfiling'); - }; + // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. + // In that case, the shell must also listen for this specific message to know when it needs to reload the app. + // The agent can't do this in a way that is renderer agnostic. + this._bridge.send('reloadAppForProfiling'); + }; renamePath: RenamePathParams => void = ({ hookID, @@ -740,17 +747,19 @@ export default class Agent extends EventEmitter<{ this.removeAllListeners(); }; - startProfiling: (recordChangeDescriptions: boolean) => void = - recordChangeDescriptions => { - this._isProfiling = true; - for (const rendererID in this._rendererInterfaces) { - const renderer = ((this._rendererInterfaces[ - (rendererID: any) - ]: any): RendererInterface); - renderer.startProfiling(recordChangeDescriptions); - } - this._bridge.send('profilingStatus', this._isProfiling); - }; + startProfiling: ({ + recordChangeDescriptions: boolean, + recordTimeline: boolean, + }) => void = ({recordChangeDescriptions, recordTimeline}) => { + this._isProfiling = true; + for (const rendererID in this._rendererInterfaces) { + const renderer = ((this._rendererInterfaces[ + (rendererID: any) + ]: any): RendererInterface); + renderer.startProfiling(recordChangeDescriptions, recordTimeline); + } + this._bridge.send('profilingStatus', this._isProfiling); + }; stopProfiling: () => void = () => { this._isProfiling = false; diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 57aca225b8..d582e2a3ce 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -5035,6 +5035,7 @@ export function attach( let isProfiling: boolean = false; let profilingStartTime: number = 0; let recordChangeDescriptions: boolean = false; + let recordTimeline: boolean = false; let rootToCommitProfilingMetadataMap: CommitProfilingMetadataMap | null = null; @@ -5176,12 +5177,16 @@ export function attach( } } - function startProfiling(shouldRecordChangeDescriptions: boolean) { + function startProfiling( + shouldRecordChangeDescriptions: boolean, + shouldRecordTimeline: boolean, + ) { if (isProfiling) { return; } recordChangeDescriptions = shouldRecordChangeDescriptions; + recordTimeline = shouldRecordTimeline; // Capture initial values as of the time profiling starts. // It's important we snapshot both the durations and the id-to-root map, @@ -5212,7 +5217,7 @@ export function attach( rootToCommitProfilingMetadataMap = new Map(); if (toggleProfilingStatus !== null) { - toggleProfilingStatus(true); + toggleProfilingStatus(true, recordTimeline); } } @@ -5221,13 +5226,18 @@ export function attach( recordChangeDescriptions = false; if (toggleProfilingStatus !== null) { - toggleProfilingStatus(false); + toggleProfilingStatus(false, recordTimeline); } + + recordTimeline = false; } // Automatically start profiling so that we don't miss timing info from initial "mount". if (shouldStartProfilingNow) { - startProfiling(profilingSettings.recordChangeDescriptions); + startProfiling( + profilingSettings.recordChangeDescriptions, + profilingSettings.recordTimeline, + ); } function getNearestFiber(devtoolsInstance: DevToolsInstance): null | Fiber { diff --git a/packages/react-devtools-shared/src/backend/profilingHooks.js b/packages/react-devtools-shared/src/backend/profilingHooks.js index 47a0103530..a8111713c9 100644 --- a/packages/react-devtools-shared/src/backend/profilingHooks.js +++ b/packages/react-devtools-shared/src/backend/profilingHooks.js @@ -97,7 +97,10 @@ export function setPerformanceMock_ONLY_FOR_TESTING( } export type GetTimelineData = () => TimelineData | null; -export type ToggleProfilingStatus = (value: boolean) => void; +export type ToggleProfilingStatus = ( + value: boolean, + recordTimeline?: boolean, +) => void; type Response = { getTimelineData: GetTimelineData, @@ -839,7 +842,10 @@ export function createProfilingHooks({ } } - function toggleProfilingStatus(value: boolean) { + function toggleProfilingStatus( + value: boolean, + recordTimeline: boolean = false, + ) { if (isProfiling !== value) { isProfiling = value; @@ -875,34 +881,45 @@ export function createProfilingHooks({ currentReactComponentMeasure = null; currentReactMeasuresStack = []; currentFiberStacks = new Map(); - currentTimelineData = { - // Session wide metadata; only collected once. - internalModuleSourceToRanges, - laneToLabelMap: laneToLabelMap || new Map(), - reactVersion, + if (recordTimeline) { + currentTimelineData = { + // Session wide metadata; only collected once. + internalModuleSourceToRanges, + laneToLabelMap: laneToLabelMap || new Map(), + reactVersion, - // Data logged by React during profiling session. - componentMeasures: [], - schedulingEvents: [], - suspenseEvents: [], - thrownErrors: [], + // Data logged by React during profiling session. + componentMeasures: [], + schedulingEvents: [], + suspenseEvents: [], + thrownErrors: [], - // Data inferred based on what React logs. - batchUIDToMeasuresMap: new Map(), - duration: 0, - laneToReactMeasureMap, - startTime: 0, + // Data inferred based on what React logs. + batchUIDToMeasuresMap: new Map(), + duration: 0, + laneToReactMeasureMap, + startTime: 0, - // Data only available in Chrome profiles. - flamechart: [], - nativeEvents: [], - networkMeasures: [], - otherUserTimingMarks: [], - snapshots: [], - snapshotHeight: 0, - }; + // Data only available in Chrome profiles. + flamechart: [], + nativeEvents: [], + networkMeasures: [], + otherUserTimingMarks: [], + snapshots: [], + snapshotHeight: 0, + }; + } nextRenderShouldStartNewBatch = true; } else { + // This is __EXPENSIVE__. + // We could end up with hundreds of state updated, and for each one of them + // would try to create a component stack with possibly hundreds of Fibers. + // Creating a cache of component stacks won't help, generating a single stack is already expensive enough. + // We should find a way to lazily generate component stacks on demand, when user inspects a specific event. + // If we succeed with moving React DevTools Timeline Profiler to Performance panel, then Timeline Profiler would probably be removed. + // If not, then once enableOwnerStacks is adopted, revisit this again and cache component stacks per Fiber, + // but only return them when needed, sending hundreds of component stacks is beyond the Bridge's bandwidth. + // Postprocess Profile data if (currentTimelineData !== null) { currentTimelineData.schedulingEvents.forEach(event => { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 61546f2c28..9d9d9a8eb5 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -419,7 +419,10 @@ export type RendererInterface = { renderer: ReactRenderer | null, setTraceUpdatesEnabled: (enabled: boolean) => void, setTrackedPath: (path: Array | null) => void, - startProfiling: (recordChangeDescriptions: boolean) => void, + startProfiling: ( + recordChangeDescriptions: boolean, + recordTimeline: boolean, + ) => void, stopProfiling: () => void, storeAsGlobal: ( id: number, @@ -487,6 +490,7 @@ export type DevToolsBackend = { export type ProfilingSettings = { recordChangeDescriptions: boolean, + recordTimeline: boolean, }; export type DevToolsHook = { diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index dde6e7c3ff..cb494e1b3c 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -16,6 +16,7 @@ import type { ProfilingDataBackend, RendererID, DevToolsHookSettings, + ProfilingSettings, } from 'react-devtools-shared/src/backend/types'; import type {StyleAndLayout as StyleAndLayoutPayload} from 'react-devtools-shared/src/backend/NativeStyleEditor/types'; @@ -206,6 +207,9 @@ export type BackendEvents = { hookSettings: [$ReadOnly], }; +type StartProfilingParams = ProfilingSettings; +type ReloadAndProfilingParams = ProfilingSettings; + type FrontendEvents = { clearErrorsAndWarnings: [{rendererID: RendererID}], clearErrorsForElementID: [ElementAndRendererID], @@ -226,13 +230,13 @@ type FrontendEvents = { overrideSuspense: [OverrideSuspense], overrideValueAtPath: [OverrideValueAtPath], profilingData: [ProfilingDataBackend], - reloadAndProfile: [boolean], + reloadAndProfile: [ReloadAndProfilingParams], renamePath: [RenamePath], savedPreferences: [SavedPreferencesParams], setTraceUpdatesEnabled: [boolean], shutdown: [], startInspectingHost: [], - startProfiling: [boolean], + startProfiling: [StartProfilingParams], stopInspectingHost: [boolean], stopProfiling: [], storeAsGlobal: [StoreAsGlobalParams], diff --git a/packages/react-devtools-shared/src/constants.js b/packages/react-devtools-shared/src/constants.js index 6893610b46..b087381659 100644 --- a/packages/react-devtools-shared/src/constants.js +++ b/packages/react-devtools-shared/src/constants.js @@ -41,6 +41,8 @@ export const LOCAL_STORAGE_PARSE_HOOK_NAMES_KEY = 'React::DevTools::parseHookNames'; export const SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY = 'React::DevTools::recordChangeDescriptions'; +export const SESSION_STORAGE_RECORD_TIMELINE_KEY = + 'React::DevTools::recordTimeline'; export const SESSION_STORAGE_RELOAD_AND_PROFILE_KEY = 'React::DevTools::reloadAndProfile'; export const LOCAL_STORAGE_BROWSER_THEME = 'React::DevTools::theme'; diff --git a/packages/react-devtools-shared/src/devtools/ProfilerStore.js b/packages/react-devtools-shared/src/devtools/ProfilerStore.js index 5ed7e69f29..b9bbdb11df 100644 --- a/packages/react-devtools-shared/src/devtools/ProfilerStore.js +++ b/packages/react-devtools-shared/src/devtools/ProfilerStore.js @@ -191,7 +191,10 @@ export default class ProfilerStore extends EventEmitter<{ } startProfiling(): void { - this._bridge.send('startProfiling', this._store.recordChangeDescriptions); + this._bridge.send('startProfiling', { + recordChangeDescriptions: this._store.recordChangeDescriptions, + recordTimeline: this._store.supportsTimeline, + }); this._isProfilingBasedOnUserInput = true; this.emit('isProfiling'); diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js index 90dba6f7d8..95ac82a973 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js @@ -54,8 +54,11 @@ export default function ReloadAndProfileButton({ // For now, let's just skip doing it entirely to avoid paying snapshot costs for data we don't need. // startProfiling(); - bridge.send('reloadAndProfile', recordChangeDescriptions); - }, [bridge, recordChangeDescriptions]); + bridge.send('reloadAndProfile', { + recordChangeDescriptions, + recordTimeline: store.supportsTimeline, + }); + }, [bridge, recordChangeDescriptions, store]); if (!supportsReloadAndProfile) { return null; diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 4aa6518cd1..d754140f96 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -52,6 +52,7 @@ const targetConsole: Object = console; const defaultProfilingSettings: ProfilingSettings = { recordChangeDescriptions: false, + recordTimeline: false, }; export function installHook( diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index d3f18920fc..bbf1599898 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -38,6 +38,7 @@ import { LOCAL_STORAGE_OPEN_IN_EDITOR_URL, SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, + SESSION_STORAGE_RECORD_TIMELINE_KEY, } from './constants'; import { ComponentFilterElementType, @@ -1002,18 +1003,28 @@ export function getProfilingSettings(): ProfilingSettings { recordChangeDescriptions: sessionStorageGetItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY) === 'true', + recordTimeline: + sessionStorageGetItem(SESSION_STORAGE_RECORD_TIMELINE_KEY) === 'true', }; } -export function onReloadAndProfile(recordChangeDescriptions: boolean): void { +export function onReloadAndProfile( + recordChangeDescriptions: boolean, + recordTimeline: boolean, +): void { sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true'); sessionStorageSetItem( SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, recordChangeDescriptions ? 'true' : 'false', ); + sessionStorageSetItem( + SESSION_STORAGE_RECORD_TIMELINE_KEY, + recordTimeline ? 'true' : 'false', + ); } export function onReloadAndProfileFlagsReset(): void { sessionStorageRemoveItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY); sessionStorageRemoveItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY); + sessionStorageRemoveItem(SESSION_STORAGE_RECORD_TIMELINE_KEY); }