@samitouri / QOS-React / commits / d5bba18b5d

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.

Ruslan Lesiutin committed Oct 9, 2024 at 15:27 UTC d5bba18b5d81f234657586865248c5b6849599cd

10 files changed +126 -62

packages/react-devtools-shared/src/backend/agent.js

+32 -23

@@ -153,12 +153,17 @@ export default class Agent extends EventEmitter<{

153 _persistedSelection: PersistedSelection | null = null;

154 _persistedSelectionMatch: PathMatch | null = null;

155 _traceUpdatesEnabled: boolean = false;

156 - _onReloadAndProfile: ((recordChangeDescriptions: boolean) => void) | void;

156 + _onReloadAndProfile:

157 + | ((recordChangeDescriptions: boolean, recordTimeline: boolean) => void)

158 + | void;

159

160 constructor(

161 bridge: BackendBridge,

162 isProfiling: boolean = false,

161 - onReloadAndProfile?: (recordChangeDescriptions: boolean) => void,

163 + onReloadAndProfile?: (

164 + recordChangeDescriptions: boolean,

165 + recordTimeline: boolean,

166 + ) => void,

167 ) {

168 super();

169

@@ -658,17 +663,19 @@ export default class Agent extends EventEmitter<{

663 this._bridge.send('isReloadAndProfileSupportedByBackend', true);

664 };

665

661 - reloadAndProfile: (recordChangeDescriptions: boolean) => void =

662 - recordChangeDescriptions => {

663 - if (typeof this._onReloadAndProfile === 'function') {

664 - this._onReloadAndProfile(recordChangeDescriptions);

665 - }

666 + reloadAndProfile: ({

667 + recordChangeDescriptions: boolean,

668 + recordTimeline: boolean,

669 + }) => void = ({recordChangeDescriptions, recordTimeline}) => {

670 + if (typeof this._onReloadAndProfile === 'function') {

671 + this._onReloadAndProfile(recordChangeDescriptions, recordTimeline);

672 + }

673

667 - // This code path should only be hit if the shell has explicitly told the Store that it supports profiling.

668 - // In that case, the shell must also listen for this specific message to know when it needs to reload the app.

669 - // The agent can't do this in a way that is renderer agnostic.

670 - this._bridge.send('reloadAppForProfiling');

671 - };

674 + // This code path should only be hit if the shell has explicitly told the Store that it supports profiling.

675 + // In that case, the shell must also listen for this specific message to know when it needs to reload the app.

676 + // The agent can't do this in a way that is renderer agnostic.

677 + this._bridge.send('reloadAppForProfiling');

678 + };

679

680 renamePath: RenamePathParams => void = ({

681 hookID,

@@ -740,17 +747,19 @@ export default class Agent extends EventEmitter<{

747 this.removeAllListeners();

748 };

749

743 - startProfiling: (recordChangeDescriptions: boolean) => void =

744 - recordChangeDescriptions => {

745 - this._isProfiling = true;

746 - for (const rendererID in this._rendererInterfaces) {

747 - const renderer = ((this._rendererInterfaces[

748 - (rendererID: any)

749 - ]: any): RendererInterface);

750 - renderer.startProfiling(recordChangeDescriptions);

751 - }

752 - this._bridge.send('profilingStatus', this._isProfiling);

753 - };

750 + startProfiling: ({

751 + recordChangeDescriptions: boolean,

752 + recordTimeline: boolean,

753 + }) => void = ({recordChangeDescriptions, recordTimeline}) => {

754 + this._isProfiling = true;

755 + for (const rendererID in this._rendererInterfaces) {

756 + const renderer = ((this._rendererInterfaces[

757 + (rendererID: any)

758 + ]: any): RendererInterface);

759 + renderer.startProfiling(recordChangeDescriptions, recordTimeline);

760 + }

761 + this._bridge.send('profilingStatus', this._isProfiling);

762 + };

763

764 stopProfiling: () => void = () => {

765 this._isProfiling = false;

packages/react-devtools-shared/src/backend/fiber/renderer.js

+14 -4

@@ -5035,6 +5035,7 @@ export function attach(

5035 let isProfiling: boolean = false;

5036 let profilingStartTime: number = 0;

5037 let recordChangeDescriptions: boolean = false;

5038 + let recordTimeline: boolean = false;

5039 let rootToCommitProfilingMetadataMap: CommitProfilingMetadataMap | null =

5040 null;

5041

@@ -5176,12 +5177,16 @@ export function attach(

5177 }

5178 }

5179

5179 - function startProfiling(shouldRecordChangeDescriptions: boolean) {

5180 + function startProfiling(

5181 + shouldRecordChangeDescriptions: boolean,

5182 + shouldRecordTimeline: boolean,

5183 + ) {

5184 if (isProfiling) {

5185 return;

5186 }

5187

5188 recordChangeDescriptions = shouldRecordChangeDescriptions;

5189 + recordTimeline = shouldRecordTimeline;

5190

5191 // Capture initial values as of the time profiling starts.

5192 // It's important we snapshot both the durations and the id-to-root map,

@@ -5212,7 +5217,7 @@ export function attach(

5217 rootToCommitProfilingMetadataMap = new Map();

5218

5219 if (toggleProfilingStatus !== null) {

5215 - toggleProfilingStatus(true);

5220 + toggleProfilingStatus(true, recordTimeline);

5221 }

5222 }

5223

@@ -5221,13 +5226,18 @@ export function attach(

5226 recordChangeDescriptions = false;

5227

5228 if (toggleProfilingStatus !== null) {

5224 - toggleProfilingStatus(false);

5229 + toggleProfilingStatus(false, recordTimeline);

5230 }

5231 +

5232 + recordTimeline = false;

5233 }

5234

5235 // Automatically start profiling so that we don't miss timing info from initial "mount".

5236 if (shouldStartProfilingNow) {

5230 - startProfiling(profilingSettings.recordChangeDescriptions);

5237 + startProfiling(

5238 + profilingSettings.recordChangeDescriptions,

5239 + profilingSettings.recordTimeline,

5240 + );

5241 }

5242

5243 function getNearestFiber(devtoolsInstance: DevToolsInstance): null | Fiber {

packages/react-devtools-shared/src/backend/profilingHooks.js

+45 -28

@@ -97,7 +97,10 @@ export function setPerformanceMock_ONLY_FOR_TESTING(

97 }

99 export type GetTimelineData = () => TimelineData | null;

100 -export type ToggleProfilingStatus = (value: boolean) => void;

100 +export type ToggleProfilingStatus = (

101 + value: boolean,

102 + recordTimeline?: boolean,

103 +) => void;

104

105 type Response = {

106 getTimelineData: GetTimelineData,

@@ -839,7 +842,10 @@ export function createProfilingHooks({

842 }

843 }

844

842 - function toggleProfilingStatus(value: boolean) {

845 + function toggleProfilingStatus(

846 + value: boolean,

847 + recordTimeline: boolean = false,

848 + ) {

849 if (isProfiling !== value) {

850 isProfiling = value;

851

@@ -875,34 +881,45 @@ export function createProfilingHooks({

881 currentReactComponentMeasure = null;

882 currentReactMeasuresStack = [];

883 currentFiberStacks = new Map();

878 - currentTimelineData = {

879 - // Session wide metadata; only collected once.

880 - internalModuleSourceToRanges,

881 - laneToLabelMap: laneToLabelMap || new Map(),

882 - reactVersion,

883 -

884 - // Data logged by React during profiling session.

885 - componentMeasures: [],

886 - schedulingEvents: [],

887 - suspenseEvents: [],

888 - thrownErrors: [],

889 -

890 - // Data inferred based on what React logs.

891 - batchUIDToMeasuresMap: new Map(),

892 - duration: 0,

893 - laneToReactMeasureMap,

894 - startTime: 0,

895 -

896 - // Data only available in Chrome profiles.

897 - flamechart: [],

898 - nativeEvents: [],

899 - networkMeasures: [],

900 - otherUserTimingMarks: [],

901 - snapshots: [],

902 - snapshotHeight: 0,

903 - };

884 + if (recordTimeline) {

885 + currentTimelineData = {

886 + // Session wide metadata; only collected once.

887 + internalModuleSourceToRanges,

888 + laneToLabelMap: laneToLabelMap || new Map(),

889 + reactVersion,

890 +

891 + // Data logged by React during profiling session.

892 + componentMeasures: [],

893 + schedulingEvents: [],

894 + suspenseEvents: [],

895 + thrownErrors: [],

896 +

897 + // Data inferred based on what React logs.

898 + batchUIDToMeasuresMap: new Map(),

899 + duration: 0,

900 + laneToReactMeasureMap,

901 + startTime: 0,

902 +

903 + // Data only available in Chrome profiles.

904 + flamechart: [],

905 + nativeEvents: [],

906 + networkMeasures: [],

907 + otherUserTimingMarks: [],

908 + snapshots: [],

909 + snapshotHeight: 0,

910 + };

911 + }

912 nextRenderShouldStartNewBatch = true;

913 } else {

914 + // This is __EXPENSIVE__.

915 + // We could end up with hundreds of state updated, and for each one of them

916 + // would try to create a component stack with possibly hundreds of Fibers.

917 + // Creating a cache of component stacks won't help, generating a single stack is already expensive enough.

918 + // We should find a way to lazily generate component stacks on demand, when user inspects a specific event.

919 + // If we succeed with moving React DevTools Timeline Profiler to Performance panel, then Timeline Profiler would probably be removed.

920 + // If not, then once enableOwnerStacks is adopted, revisit this again and cache component stacks per Fiber,

921 + // but only return them when needed, sending hundreds of component stacks is beyond the Bridge's bandwidth.

922 +

923 // Postprocess Profile data

924 if (currentTimelineData !== null) {

925 currentTimelineData.schedulingEvents.forEach(event => {

packages/react-devtools-shared/src/backend/types.js

+5 -1

@@ -419,7 +419,10 @@ export type RendererInterface = {

419 renderer: ReactRenderer | null,

420 setTraceUpdatesEnabled: (enabled: boolean) => void,

421 setTrackedPath: (path: Array<PathFrame> | null) => void,

422 - startProfiling: (recordChangeDescriptions: boolean) => void,

422 + startProfiling: (

423 + recordChangeDescriptions: boolean,

424 + recordTimeline: boolean,

425 + ) => void,

426 stopProfiling: () => void,

427 storeAsGlobal: (

428 id: number,

@@ -487,6 +490,7 @@ export type DevToolsBackend = {

490

491 export type ProfilingSettings = {

492 recordChangeDescriptions: boolean,

493 + recordTimeline: boolean,

494 };

495

496 export type DevToolsHook = {

packages/react-devtools-shared/src/bridge.js

+6 -2

@@ -16,6 +16,7 @@ import type {

16 ProfilingDataBackend,

17 RendererID,

18 DevToolsHookSettings,

19 + ProfilingSettings,

20 } from 'react-devtools-shared/src/backend/types';

21 import type {StyleAndLayout as StyleAndLayoutPayload} from 'react-devtools-shared/src/backend/NativeStyleEditor/types';

@@ -206,6 +207,9 @@ export type BackendEvents = {

207 hookSettings: [$ReadOnly<DevToolsHookSettings>],

208 };

209

210 +type StartProfilingParams = ProfilingSettings;

211 +type ReloadAndProfilingParams = ProfilingSettings;

212 +

213 type FrontendEvents = {

214 clearErrorsAndWarnings: [{rendererID: RendererID}],

215 clearErrorsForElementID: [ElementAndRendererID],

@@ -226,13 +230,13 @@ type FrontendEvents = {

230 overrideSuspense: [OverrideSuspense],

231 overrideValueAtPath: [OverrideValueAtPath],

232 profilingData: [ProfilingDataBackend],

229 - reloadAndProfile: [boolean],

233 + reloadAndProfile: [ReloadAndProfilingParams],

234 renamePath: [RenamePath],

235 savedPreferences: [SavedPreferencesParams],

236 setTraceUpdatesEnabled: [boolean],

237 shutdown: [],

238 startInspectingHost: [],

235 - startProfiling: [boolean],

239 + startProfiling: [StartProfilingParams],

240 stopInspectingHost: [boolean],

241 stopProfiling: [],

242 storeAsGlobal: [StoreAsGlobalParams],

packages/react-devtools-shared/src/constants.js

@@ -41,6 +41,8 @@ export const LOCAL_STORAGE_PARSE_HOOK_NAMES_KEY =

41 'React::DevTools::parseHookNames';

42 export const SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY =

43 'React::DevTools::recordChangeDescriptions';

44 +export const SESSION_STORAGE_RECORD_TIMELINE_KEY =

45 + 'React::DevTools::recordTimeline';

46 export const SESSION_STORAGE_RELOAD_AND_PROFILE_KEY =

47 'React::DevTools::reloadAndProfile';

48 export const LOCAL_STORAGE_BROWSER_THEME = 'React::DevTools::theme';

packages/react-devtools-shared/src/devtools/ProfilerStore.js

+4 -1

@@ -191,7 +191,10 @@ export default class ProfilerStore extends EventEmitter<{

191 }

192

193 startProfiling(): void {

194 - this._bridge.send('startProfiling', this._store.recordChangeDescriptions);

194 + this._bridge.send('startProfiling', {

195 + recordChangeDescriptions: this._store.recordChangeDescriptions,

196 + recordTimeline: this._store.supportsTimeline,

197 + });

198

199 this._isProfilingBasedOnUserInput = true;

200 this.emit('isProfiling');

packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js

+5 -2

@@ -54,8 +54,11 @@ export default function ReloadAndProfileButton({

54 // For now, let's just skip doing it entirely to avoid paying snapshot costs for data we don't need.

55 // startProfiling();

57 - bridge.send('reloadAndProfile', recordChangeDescriptions);

58 - }, [bridge, recordChangeDescriptions]);

57 + bridge.send('reloadAndProfile', {

58 + recordChangeDescriptions,

59 + recordTimeline: store.supportsTimeline,

60 + });

61 + }, [bridge, recordChangeDescriptions, store]);

63 if (!supportsReloadAndProfile) {

64 return null;

packages/react-devtools-shared/src/hook.js

@@ -52,6 +52,7 @@ const targetConsole: Object = console;

53 const defaultProfilingSettings: ProfilingSettings = {

54 recordChangeDescriptions: false,

55 + recordTimeline: false,

56 };

58 export function installHook(

packages/react-devtools-shared/src/utils.js

+12 -1

@@ -38,6 +38,7 @@ import {

38 LOCAL_STORAGE_OPEN_IN_EDITOR_URL,

39 SESSION_STORAGE_RELOAD_AND_PROFILE_KEY,

40 SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY,

41 + SESSION_STORAGE_RECORD_TIMELINE_KEY,

42 } from './constants';

43 import {

44 ComponentFilterElementType,

@@ -1002,18 +1003,28 @@ export function getProfilingSettings(): ProfilingSettings {

1003 recordChangeDescriptions:

1004 sessionStorageGetItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY) ===

1005 'true',

1006 + recordTimeline:

1007 + sessionStorageGetItem(SESSION_STORAGE_RECORD_TIMELINE_KEY) === 'true',

1008 };

1009 }

1010

1008 -export function onReloadAndProfile(recordChangeDescriptions: boolean): void {

1011 +export function onReloadAndProfile(

1012 + recordChangeDescriptions: boolean,

1013 + recordTimeline: boolean,

1014 +): void {

1015 sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true');

1016 sessionStorageSetItem(

1017 SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY,

1018 recordChangeDescriptions ? 'true' : 'false',

1019 );

1020 + sessionStorageSetItem(

1021 + SESSION_STORAGE_RECORD_TIMELINE_KEY,

1022 + recordTimeline ? 'true' : 'false',

1023 + );

1024 }

1025

1026 export function onReloadAndProfileFlagsReset(): void {

1027 sessionStorageRemoveItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY);

1028 sessionStorageRemoveItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY);

1029 + sessionStorageRemoveItem(SESSION_STORAGE_RECORD_TIMELINE_KEY);

1030 }