From 9b35dd2fcc8b8dfbd1363cef9e5c59a0deab0dd3 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 14 Aug 2020 15:21:13 -0400 Subject: [PATCH] Permanently removed component stacks from scheduling profiler data (#19615) These stacks improve the profiler data but they're expensive to generate and generating them can also cause runtime errors in larger applications (although an exact repro has been hard to nail down). Removing them for now. We can revisit adding them after this profiler has been integrated into the DevTools extension and we can generate them lazily. --- .../src/SchedulingProfiler.js | 61 ++------- .../SchedulingProfiler-test.internal.js | 129 ++++-------------- packages/shared/ReactFeatureFlags.js | 1 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - .../ReactFeatureFlags.test-renderer.native.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../shared/forks/ReactFeatureFlags.testing.js | 1 - .../forks/ReactFeatureFlags.testing.www.js | 1 - .../forks/ReactFeatureFlags.www-dynamic.js | 6 - .../shared/forks/ReactFeatureFlags.www.js | 1 - 12 files changed, 33 insertions(+), 172 deletions(-) diff --git a/packages/react-reconciler/src/SchedulingProfiler.js b/packages/react-reconciler/src/SchedulingProfiler.js index 7e06b9821600a..4c2a18b780e68 100644 --- a/packages/react-reconciler/src/SchedulingProfiler.js +++ b/packages/react-reconciler/src/SchedulingProfiler.js @@ -11,13 +11,9 @@ import type {Lane, Lanes} from './ReactFiberLane'; import type {Fiber} from './ReactInternalTypes'; import type {Wakeable} from 'shared/ReactTypes'; -import { - enableSchedulingProfiler, - enableSchedulingProfilerComponentStacks, -} from 'shared/ReactFeatureFlags'; +import {enableSchedulingProfiler} from 'shared/ReactFeatureFlags'; import ReactVersion from 'shared/ReactVersion'; import getComponentName from 'shared/getComponentName'; -import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack'; /** * If performance exists and supports the subset of the User Timing API that we @@ -65,51 +61,16 @@ function getWakeableID(wakeable: Wakeable): number { return ((wakeableIDs.get(wakeable): any): number); } -let getComponentStackByFiber = function getComponentStackByFiberDisabled( - fiber: Fiber, -): string { - return ''; -}; - -if (enableSchedulingProfilerComponentStacks) { - // $FlowFixMe: Flow cannot handle polymorphic WeakMaps - const cachedFiberStacks: WeakMap = new PossiblyWeakMap(); - getComponentStackByFiber = function cacheFirstGetComponentStackByFiber( - fiber: Fiber, - ): string { - if (cachedFiberStacks.has(fiber)) { - return ((cachedFiberStacks.get(fiber): any): string); - } else { - const alternate = fiber.alternate; - if (alternate !== null && cachedFiberStacks.has(alternate)) { - return ((cachedFiberStacks.get(alternate): any): string); - } - } - // TODO (brian) Generate and store temporary ID so DevTools can match up a component stack later. - const componentStack = getStackByFiberInDevAndProd(fiber) || ''; - cachedFiberStacks.set(fiber, componentStack); - return componentStack; - }; -} - export function markComponentSuspended(fiber: Fiber, wakeable: Wakeable): void { if (enableSchedulingProfiler) { if (supportsUserTiming) { const id = getWakeableID(wakeable); const componentName = getComponentName(fiber.type) || 'Unknown'; - const componentStack = getComponentStackByFiber(fiber); - performance.mark( - `--suspense-suspend-${id}-${componentName}-${componentStack}`, - ); + // TODO Add component stack id + performance.mark(`--suspense-suspend-${id}-${componentName}`); wakeable.then( - () => - performance.mark( - `--suspense-resolved-${id}-${componentName}-${componentStack}`, - ), - () => - performance.mark( - `--suspense-rejected-${id}-${componentName}-${componentStack}`, - ), + () => performance.mark(`--suspense-resolved-${id}-${componentName}`), + () => performance.mark(`--suspense-rejected-${id}-${componentName}`), ); } } @@ -183,11 +144,9 @@ export function markForceUpdateScheduled(fiber: Fiber, lane: Lane): void { if (enableSchedulingProfiler) { if (supportsUserTiming) { const componentName = getComponentName(fiber.type) || 'Unknown'; - const componentStack = getComponentStackByFiber(fiber); + // TODO Add component stack id performance.mark( - `--schedule-forced-update-${formatLanes( - lane, - )}-${componentName}-${componentStack}`, + `--schedule-forced-update-${formatLanes(lane)}-${componentName}`, ); } } @@ -197,11 +156,9 @@ export function markStateUpdateScheduled(fiber: Fiber, lane: Lane): void { if (enableSchedulingProfiler) { if (supportsUserTiming) { const componentName = getComponentName(fiber.type) || 'Unknown'; - const componentStack = getComponentStackByFiber(fiber); + // TODO Add component stack id performance.mark( - `--schedule-state-update-${formatLanes( - lane, - )}-${componentName}-${componentStack}`, + `--schedule-state-update-${formatLanes(lane)}-${componentName}`, ); } } diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js index ce6bc2ac0904c..79580d17202fd 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js @@ -12,29 +12,6 @@ import ReactVersion from 'shared/ReactVersion'; -function normalizeCodeLocInfo(str) { - return ( - str && - str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) { - return '\n in ' + name + ' (at **)'; - }) - ); -} - -// TODO (enableSchedulingProfilerComponentStacks) Clean this up once the feature flag has been removed. -function toggleComponentStacks(mark) { - let expectedMark = mark; - gate(({enableSchedulingProfilerComponentStacks}) => { - if (!enableSchedulingProfilerComponentStacks) { - const index = mark.indexOf('\n '); - if (index >= 0) { - expectedMark = mark.substr(0, index); - } - } - }); - return expectedMark; -} - describe('SchedulingProfiler', () => { let React; let ReactTestRenderer; @@ -162,9 +139,7 @@ describe('SchedulingProfiler', () => { `--react-init-${ReactVersion}`, '--schedule-render-1', '--render-start-1', - toggleComponentStacks( - '--suspense-suspend-0-Example-\n at Example\n at Suspense', - ), + '--suspense-suspend-0-Example', '--render-stop', '--commit-start-1', '--layout-effects-start-1', @@ -175,11 +150,7 @@ describe('SchedulingProfiler', () => { marks.splice(0); await fakeSuspensePromise; - expect(marks).toEqual([ - toggleComponentStacks( - '--suspense-resolved-0-Example-\n at Example\n at Suspense', - ), - ]); + expect(marks).toEqual(['--suspense-resolved-0-Example']); }); // @gate enableSchedulingProfiler @@ -199,9 +170,7 @@ describe('SchedulingProfiler', () => { `--react-init-${ReactVersion}`, '--schedule-render-1', '--render-start-1', - toggleComponentStacks( - '--suspense-suspend-0-Example-\n at Example\n at Suspense', - ), + '--suspense-suspend-0-Example', '--render-stop', '--commit-start-1', '--layout-effects-start-1', @@ -212,11 +181,7 @@ describe('SchedulingProfiler', () => { marks.splice(0); await expect(fakeSuspensePromise).rejects.toThrow(); - expect(marks).toEqual([ - toggleComponentStacks( - '--suspense-rejected-0-Example-\n at Example\n at Suspense', - ), - ]); + expect(marks).toEqual(['--suspense-rejected-0-Example']); }); // @gate enableSchedulingProfiler @@ -244,9 +209,7 @@ describe('SchedulingProfiler', () => { expect(marks).toEqual([ '--render-start-512', - toggleComponentStacks( - '--suspense-suspend-0-Example-\n at Example\n at Suspense', - ), + '--suspense-suspend-0-Example', '--render-stop', '--commit-start-512', '--layout-effects-start-512', @@ -257,11 +220,7 @@ describe('SchedulingProfiler', () => { marks.splice(0); await fakeSuspensePromise; - expect(marks).toEqual([ - toggleComponentStacks( - '--suspense-resolved-0-Example-\n at Example\n at Suspense', - ), - ]); + expect(marks).toEqual(['--suspense-resolved-0-Example']); }); // @gate enableSchedulingProfiler @@ -289,9 +248,7 @@ describe('SchedulingProfiler', () => { expect(marks).toEqual([ '--render-start-512', - toggleComponentStacks( - '--suspense-suspend-0-Example-\n at Example\n at Suspense', - ), + '--suspense-suspend-0-Example', '--render-stop', '--commit-start-512', '--layout-effects-start-512', @@ -302,11 +259,7 @@ describe('SchedulingProfiler', () => { marks.splice(0); await expect(fakeSuspensePromise).rejects.toThrow(); - expect(marks).toEqual([ - toggleComponentStacks( - '--suspense-rejected-0-Example-\n at Example\n at Suspense', - ), - ]); + expect(marks).toEqual(['--suspense-rejected-0-Example']); }); // @gate enableSchedulingProfiler @@ -332,14 +285,12 @@ describe('SchedulingProfiler', () => { expect(Scheduler).toFlushUntilNextPaint([]); - expect(marks.map(normalizeCodeLocInfo)).toEqual([ + expect(marks).toEqual([ '--render-start-512', '--render-stop', '--commit-start-512', '--layout-effects-start-512', - toggleComponentStacks( - '--schedule-state-update-1-Example-\n in Example (at **)', - ), + '--schedule-state-update-1-Example', '--layout-effects-stop', '--render-start-1', '--render-stop', @@ -371,14 +322,12 @@ describe('SchedulingProfiler', () => { expect(Scheduler).toFlushUntilNextPaint([]); - expect(marks.map(normalizeCodeLocInfo)).toEqual([ + expect(marks).toEqual([ '--render-start-512', '--render-stop', '--commit-start-512', '--layout-effects-start-512', - toggleComponentStacks( - '--schedule-forced-update-1-Example-\n in Example (at **)', - ), + '--schedule-forced-update-1-Example', '--layout-effects-stop', '--render-start-1', '--render-stop', @@ -415,16 +364,8 @@ describe('SchedulingProfiler', () => { gate(({old}) => old - ? expect(marks.map(normalizeCodeLocInfo)).toContain( - toggleComponentStacks( - '--schedule-state-update-1024-Example-\n in Example (at **)', - ), - ) - : expect(marks.map(normalizeCodeLocInfo)).toContain( - toggleComponentStacks( - '--schedule-state-update-512-Example-\n in Example (at **)', - ), - ), + ? expect(marks).toContain('--schedule-state-update-1024-Example') + : expect(marks).toContain('--schedule-state-update-512-Example'), ); }); @@ -455,16 +396,8 @@ describe('SchedulingProfiler', () => { gate(({old}) => old - ? expect(marks.map(normalizeCodeLocInfo)).toContain( - toggleComponentStacks( - '--schedule-forced-update-1024-Example-\n in Example (at **)', - ), - ) - : expect(marks.map(normalizeCodeLocInfo)).toContain( - toggleComponentStacks( - '--schedule-forced-update-512-Example-\n in Example (at **)', - ), - ), + ? expect(marks).toContain('--schedule-forced-update-1024-Example') + : expect(marks).toContain('--schedule-forced-update-512-Example'), ); }); @@ -489,14 +422,12 @@ describe('SchedulingProfiler', () => { expect(Scheduler).toFlushUntilNextPaint([]); - expect(marks.map(normalizeCodeLocInfo)).toEqual([ + expect(marks).toEqual([ '--render-start-512', '--render-stop', '--commit-start-512', '--layout-effects-start-512', - toggleComponentStacks( - '--schedule-state-update-1-Example-\n in Example (at **)', - ), + '--schedule-state-update-1-Example', '--layout-effects-stop', '--render-start-1', '--render-stop', @@ -522,7 +453,7 @@ describe('SchedulingProfiler', () => { gate(({old}) => { if (old) { - expect(marks.map(normalizeCodeLocInfo)).toEqual([ + expect(marks).toEqual([ `--react-init-${ReactVersion}`, '--schedule-render-512', '--render-start-512', @@ -532,9 +463,7 @@ describe('SchedulingProfiler', () => { '--layout-effects-stop', '--commit-stop', '--passive-effects-start-512', - toggleComponentStacks( - '--schedule-state-update-1024-Example-\n in Example (at **)', - ), + '--schedule-state-update-1024-Example', '--passive-effects-stop', '--render-start-1024', '--render-stop', @@ -542,7 +471,7 @@ describe('SchedulingProfiler', () => { '--commit-stop', ]); } else { - expect(marks.map(normalizeCodeLocInfo)).toEqual([ + expect(marks).toEqual([ `--react-init-${ReactVersion}`, '--schedule-render-512', '--render-start-512', @@ -552,9 +481,7 @@ describe('SchedulingProfiler', () => { '--layout-effects-stop', '--commit-stop', '--passive-effects-start-512', - toggleComponentStacks( - '--schedule-state-update-1024-Example-\n in Example (at **)', - ), + '--schedule-state-update-1024-Example', '--passive-effects-stop', '--render-start-1024', '--render-stop', @@ -583,16 +510,8 @@ describe('SchedulingProfiler', () => { gate(({old}) => old - ? expect(marks.map(normalizeCodeLocInfo)).toContain( - toggleComponentStacks( - '--schedule-state-update-1024-Example-\n in Example (at **)', - ), - ) - : expect(marks.map(normalizeCodeLocInfo)).toContain( - toggleComponentStacks( - '--schedule-state-update-512-Example-\n in Example (at **)', - ), - ), + ? expect(marks).toContain('--schedule-state-update-1024-Example') + : expect(marks).toContain('--schedule-state-update-512-Example'), ); }); }); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 5ea7d961b4fd3..634c19b14ea52 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -18,7 +18,6 @@ export const enableDebugTracing = false; // Adds user timing marks for e.g. state updates, suspense, and work loop stuff, // for an experimental scheduling profiler tool. export const enableSchedulingProfiler = __PROFILE__ && __EXPERIMENTAL__; -export const enableSchedulingProfilerComponentStacks = false; // Helps identify side effects in render-phase lifecycle hooks and setState // reducers by double invoking them in Strict Mode. diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index ffab7e2c44e35..664a11a43b981 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.native-fb'; // The rest of the flags are static for better dead code elimination. export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const enableProfilerTimer = __PROFILE__; export const enableProfilerCommitHooks = false; export const enableSchedulerTracing = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index ac3e17527db00..9632b8b7da08b 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.native-oss'; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const warnAboutDeprecatedLifecycles = true; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index d436c9e1d67c7..6f348ed149b22 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.test-renderer'; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const warnAboutDeprecatedLifecycles = true; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 99b14ee92e77f..ea8f7d4e0a97e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.test-renderer'; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const warnAboutDeprecatedLifecycles = true; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index bc06ea7359416..3e5046f0fccd8 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.test-renderer.www'; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const warnAboutDeprecatedLifecycles = true; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 09e5193473d4f..8d4fe939dc09b 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.testing'; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const warnAboutDeprecatedLifecycles = true; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 2dea66f08fb23..d3796937a3028 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -13,7 +13,6 @@ import typeof * as ExportsType from './ReactFeatureFlags.testing.www'; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableDebugTracing = false; export const enableSchedulingProfiler = false; -export const enableSchedulingProfilerComponentStacks = false; export const warnAboutDeprecatedLifecycles = true; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 201a40fdbfa5b..9b23645a3d87e 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -25,12 +25,6 @@ export const decoupleUpdatePriorityFromScheduler = __VARIANT__; // NOTE: This feature will only work in DEV mode; all callsights are wrapped with __DEV__. export const enableDebugTracing = false; -// TODO: getStackByFiberInDevAndProd() causes errors when synced to www. -// This flag can be used to disable component stacks for the profiler marks, -// so that the feature can be synced for others, -// while still enabling investigation into the underlying source of the errors. -export const enableSchedulingProfilerComponentStacks = false; - // This only has an effect in the new reconciler. But also, the new reconciler // is only enabled when __VARIANT__ is true. So this is set to the opposite of // __VARIANT__ so that it's `false` when running against the new reconciler. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index bb52cc86083a4..1c637762fe6b9 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -26,7 +26,6 @@ export const { deferRenderPhaseUpdateToNextBatch, decoupleUpdatePriorityFromScheduler, enableDebugTracing, - enableSchedulingProfilerComponentStacks, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.