Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scheduling Profiler: De-emphasize React internal frames #22588

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@

declare var chrome: any;

import {__DEBUG__} from 'react-devtools-shared/src/constants';
import {
INTERNAL_EXTENSION_ID,
LOCAL_EXTENSION_ID,
__DEBUG__,
} from 'react-devtools-shared/src/constants';
import {getBrowserName} from './utils';
import {
EXTENSION_INSTALL_CHECK,
EXTENSION_INSTALLATION_TYPE,
INTERNAL_EXTENSION_ID,
LOCAL_EXTENSION_ID,
} from './constants';

const IS_CHROME = getBrowserName() === 'Chrome';
Expand Down
10 changes: 6 additions & 4 deletions packages/react-devtools-extensions/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
* @flow strict-local
*/

import {
CHROME_WEBSTORE_EXTENSION_ID,
INTERNAL_EXTENSION_ID,
LOCAL_EXTENSION_ID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason to move these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So they can be accessed by the isInternalModule check also.

} from 'react-devtools-shared/src/constants';

declare var chrome: any;

export const CURRENT_EXTENSION_ID = chrome.runtime.id;
Expand All @@ -15,10 +21,6 @@ export const EXTENSION_INSTALL_CHECK = 'extension-install-check';
export const SHOW_DUPLICATE_EXTENSION_WARNING =
'show-duplicate-extension-warning';

export const CHROME_WEBSTORE_EXTENSION_ID = 'fmkadmapgofadopljbjfkapdkoienihi';
export const INTERNAL_EXTENSION_ID = 'dnjnjgbfilfphmojnmhliehogmojhclc';
export const LOCAL_EXTENSION_ID = 'ikiahnapldjmdmpkmfhjdjilojjhgcbf';

export const EXTENSION_INSTALLATION_TYPE:
| 'public'
| 'internal'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ function AutoSizedCanvas({
surface,
defaultFrame,
data.flamechart,
data.internalModuleSourceToRanges,
data.duration,
);
flamechartViewRef.current = flamechartView;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type {
Flamechart,
FlamechartStackFrame,
FlamechartStackLayer,
InternalModuleSourceToRanges,
} from '../types';
import type {
Interaction,
Expand All @@ -20,6 +21,11 @@ import type {
ViewRefs,
} from '../view-base';

import {
CHROME_WEBSTORE_EXTENSION_ID,
INTERNAL_EXTENSION_ID,
LOCAL_EXTENSION_ID,
} from 'react-devtools-shared/src/constants';
import {
BackgroundColorView,
Surface,
Expand Down Expand Up @@ -69,13 +75,65 @@ function hoverColorForStackFrame(stackFrame: FlamechartStackFrame): string {
return hslaColorToString(color);
}

function isInternalModule(
internalModuleSourceToRanges: InternalModuleSourceToRanges,
flamechartStackFrame: FlamechartStackFrame,
): boolean {
const {locationColumn, locationLine, scriptUrl} = flamechartStackFrame;

if (scriptUrl == null || locationColumn == null || locationLine == null) {
return true;
}

// Internal modules are only registered if DevTools was running when the profile was captured,
// but DevTools should also hide its own frames to avoid over-emphasizing them.
if (
// Handle webpack-internal:// sources
scriptUrl.includes('/react-devtools') ||
scriptUrl.includes('/react_devtools') ||
// Filter out known extension IDs
scriptUrl.includes(CHROME_WEBSTORE_EXTENSION_ID) ||
scriptUrl.includes(INTERNAL_EXTENSION_ID) ||
scriptUrl.includes(LOCAL_EXTENSION_ID)

// Unfortunately this won't get everything, like relatively loaded chunks or Web Worker files.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open for suggestions here, although doesn't seem like a blocker.

) {
return true;
}

// Filter out React internal packages.
const ranges = internalModuleSourceToRanges.get(scriptUrl);
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
if (ranges != null) {
for (let i = 0; i < ranges.length; i++) {
const [startStackFrame, stopStackFrame] = ranges[i];

const isAfterStart =
locationLine > startStackFrame.lineNumber ||
(locationLine === startStackFrame.lineNumber &&
locationColumn >= startStackFrame.columnNumber);
const isBeforeStop =
locationLine < stopStackFrame.lineNumber ||
(locationLine === stopStackFrame.lineNumber &&
locationColumn <= stopStackFrame.columnNumber);

if (isAfterStart && isBeforeStop) {
return true;
}
}
}

return false;
}

class FlamechartStackLayerView extends View {
/** Layer to display */
_stackLayer: FlamechartStackLayer;

/** A set of `stackLayer`'s frames, for efficient lookup. */
_stackFrameSet: Set<FlamechartStackFrame>;

_internalModuleSourceToRanges: InternalModuleSourceToRanges;

_intrinsicSize: Size;

_hoveredStackFrame: FlamechartStackFrame | null = null;
Expand All @@ -85,11 +143,13 @@ class FlamechartStackLayerView extends View {
surface: Surface,
frame: Rect,
stackLayer: FlamechartStackLayer,
internalModuleSourceToRanges: InternalModuleSourceToRanges,
duration: number,
) {
super(surface, frame);
this._stackLayer = stackLayer;
this._stackFrameSet = new Set(stackLayer);
this._internalModuleSourceToRanges = internalModuleSourceToRanges;
this._intrinsicSize = {
width: duration,
height: FLAMECHART_FRAME_HEIGHT,
Expand Down Expand Up @@ -160,9 +220,19 @@ class FlamechartStackLayerView extends View {
}

const showHoverHighlight = _hoveredStackFrame === _stackLayer[i];
context.fillStyle = showHoverHighlight
? hoverColorForStackFrame(stackFrame)
: defaultColorForStackFrame(stackFrame);

let textFillStyle;
if (isInternalModule(this._internalModuleSourceToRanges, stackFrame)) {
context.fillStyle = showHoverHighlight
? COLORS.INTERNAL_MODULE_FRAME_HOVER
: COLORS.INTERNAL_MODULE_FRAME;
textFillStyle = COLORS.INTERNAL_MODULE_FRAME_TEXT;
} else {
context.fillStyle = showHoverHighlight
? hoverColorForStackFrame(stackFrame)
: defaultColorForStackFrame(stackFrame);
textFillStyle = COLORS.TEXT_COLOR;
}

const drawableRect = intersectionOfRects(nodeRect, visibleArea);
context.fillRect(
Expand All @@ -172,7 +242,9 @@ class FlamechartStackLayerView extends View {
drawableRect.size.height,
);

drawText(name, context, nodeRect, drawableRect);
drawText(name, context, nodeRect, drawableRect, {
fillStyle: textFillStyle,
});
}

// Render bottom border.
Expand Down Expand Up @@ -264,13 +336,22 @@ export class FlamechartView extends View {
surface: Surface,
frame: Rect,
flamechart: Flamechart,
internalModuleSourceToRanges: InternalModuleSourceToRanges,
duration: number,
) {
super(surface, frame, layeredLayout);
this.setDataAndUpdateSubviews(flamechart, duration);
this.setDataAndUpdateSubviews(
flamechart,
internalModuleSourceToRanges,
duration,
);
}

setDataAndUpdateSubviews(flamechart: Flamechart, duration: number) {
setDataAndUpdateSubviews(
flamechart: Flamechart,
internalModuleSourceToRanges: InternalModuleSourceToRanges,
duration: number,
) {
const {surface, frame, _onHover, _hoveredStackFrame} = this;

// Clear existing rows on data update
Expand All @@ -285,6 +366,7 @@ export class FlamechartView extends View {
surface,
frame,
stackLayer,
internalModuleSourceToRanges,
duration,
);
this._verticalStackView.addSubview(rowView);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ export const MIN_INTERVAL_SIZE_PX = 70;
// TODO Replace this with "export let" vars
export let COLORS = {
BACKGROUND: '',
INTERNAL_MODULE_FRAME: '',
INTERNAL_MODULE_FRAME_HOVER: '',
INTERNAL_MODULE_FRAME_TEXT: '',
NATIVE_EVENT: '',
NATIVE_EVENT_HOVER: '',
NETWORK_PRIMARY: '',
Expand Down Expand Up @@ -107,6 +110,15 @@ export function updateColorsToMatchTheme(element: Element): boolean {

COLORS = {
BACKGROUND: computedStyle.getPropertyValue('--color-background'),
INTERNAL_MODULE_FRAME: computedStyle.getPropertyValue(
'--color-scheduling-profiler-internal-module',
),
INTERNAL_MODULE_FRAME_HOVER: computedStyle.getPropertyValue(
'--color-scheduling-profiler-internal-module-hover',
),
INTERNAL_MODULE_FRAME_TEXT: computedStyle.getPropertyValue(
'--color-scheduling-profiler-internal-module-text',
),
NATIVE_EVENT: computedStyle.getPropertyValue(
'--color-scheduling-profiler-native-event',
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ describe('preprocessData', () => {
"componentMeasures": Array [],
"duration": 0.005,
"flamechart": Array [],
"internalModuleSourceToRanges": Map {},
"laneToLabelMap": Map {
0 => "Sync",
1 => "InputContinuousHydration",
Expand Down Expand Up @@ -449,6 +450,7 @@ describe('preprocessData', () => {
"componentMeasures": Array [],
"duration": 0.011,
"flamechart": Array [],
"internalModuleSourceToRanges": Map {},
"laneToLabelMap": Map {
0 => "Sync",
1 => "InputContinuousHydration",
Expand Down Expand Up @@ -636,6 +638,7 @@ describe('preprocessData', () => {
"componentMeasures": Array [],
"duration": 0.013,
"flamechart": Array [],
"internalModuleSourceToRanges": Map {},
"laneToLabelMap": Map {
0 => "Sync",
1 => "InputContinuousHydration",
Expand Down Expand Up @@ -914,6 +917,7 @@ describe('preprocessData', () => {
],
"duration": 0.031,
"flamechart": Array [],
"internalModuleSourceToRanges": Map {},
"laneToLabelMap": Map {
0 => "Sync",
1 => "InputContinuousHydration",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from '@elg/speedscope';
import type {TimelineEvent} from '@elg/speedscope';
import type {
ErrorStackFrame,
BatchUID,
Flamechart,
Milliseconds,
Expand All @@ -30,6 +31,7 @@ import type {
import {REACT_TOTAL_NUM_LANES, SCHEDULING_PROFILER_VERSION} from '../constants';
import InvalidProfileError from './InvalidProfileError';
import {getBatchRange} from '../utils/getBatchRange';
import ErrorStackParser from 'error-stack-parser';

type MeasureStackElement = {|
type: ReactMeasureType,
Expand All @@ -43,6 +45,8 @@ type ProcessorState = {|
asyncProcessingPromises: Promise<any>[],
batchUID: BatchUID,
currentReactComponentMeasure: ReactComponentMeasure | null,
internalModuleCurrentStackFrame: ErrorStackFrame | null,
internalModuleStackStringSet: Set<string>,
measureStack: MeasureStackElement[],
nativeEventStack: NativeEvent[],
nextRenderShouldGenerateNewBatchID: boolean,
Expand Down Expand Up @@ -793,6 +797,49 @@ function processTimelineEvent(
);
} // eslint-disable-line brace-style

// Internal module ranges
else if (name.startsWith('--react-internal-module-start-')) {
const stackFrameStart = name.substr(30);

if (!state.internalModuleStackStringSet.has(stackFrameStart)) {
state.internalModuleStackStringSet.add(stackFrameStart);

const parsedStackFrameStart = parseStackFrame(stackFrameStart);

state.internalModuleCurrentStackFrame = parsedStackFrameStart;
}
} else if (name.startsWith('--react-internal-module-stop-')) {
const stackFrameStop = name.substr(19);

if (!state.internalModuleStackStringSet.has(stackFrameStop)) {
state.internalModuleStackStringSet.add(stackFrameStop);

const parsedStackFrameStop = parseStackFrame(stackFrameStop);

if (
parsedStackFrameStop !== null &&
state.internalModuleCurrentStackFrame !== null
) {
const parsedStackFrameStart = state.internalModuleCurrentStackFrame;

state.internalModuleCurrentStackFrame = null;

const range = [parsedStackFrameStart, parsedStackFrameStop];
const ranges = currentProfilerData.internalModuleSourceToRanges.get(
parsedStackFrameStart.fileName,
);
if (ranges == null) {
currentProfilerData.internalModuleSourceToRanges.set(
parsedStackFrameStart.fileName,
[range],
);
} else {
ranges.push(range);
}
}
}
} // eslint-disable-line brace-style

// Other user timing marks/measures
else if (ph === 'R' || ph === 'n') {
// User Timing mark
Expand Down Expand Up @@ -855,6 +902,15 @@ function preprocessFlamechart(rawData: TimelineEvent[]): Flamechart {
return flamechart;
}

function parseStackFrame(stackFrame: string): ErrorStackFrame | null {
const error = new Error();
error.stack = stackFrame;

const frames = ErrorStackParser.parse(error);

return frames.length === 1 ? frames[0] : null;
}

export default async function preprocessData(
timeline: TimelineEvent[],
): Promise<ReactProfilerData> {
Expand All @@ -870,6 +926,7 @@ export default async function preprocessData(
componentMeasures: [],
duration: 0,
flamechart,
internalModuleSourceToRanges: new Map(),
laneToLabelMap: new Map(),
laneToReactMeasureMap,
nativeEvents: [],
Expand Down Expand Up @@ -913,6 +970,8 @@ export default async function preprocessData(
asyncProcessingPromises: [],
batchUID: 0,
currentReactComponentMeasure: null,
internalModuleCurrentStackFrame: null,
internalModuleStackStringSet: new Set(),
measureStack: [],
nativeEventStack: [],
nextRenderShouldGenerateNewBatchID: true,
Expand Down
Loading