fix[react-devtools]: record timeline data only when supported

Author: hoxyq

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;

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 {

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,
-
-          // 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 only available in Chrome profiles.
-          flamechart: [],
-          nativeEvents: [],
-          networkMeasures: [],
-          otherUserTimingMarks: [],
-          snapshots: [],
-          snapshotHeight: 0,
-        };
+        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 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,
+          };
+        }
         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 => {

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<PathFrame> | 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 = {

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<DevToolsHookSettings>],
 };
 
+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],

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';

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');

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;

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(

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);
 }