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 all commits
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 @@ -30,6 +31,7 @@ import {
rectIntersectsRect,
verticallyStackedLayout,
} from '../view-base';
import {isInternalModule} from './utils/moduleFilters';
import {
durationToWidth,
positioningScaleFactor,
Expand Down Expand Up @@ -76,6 +78,8 @@ class FlamechartStackLayerView extends View {
/** A set of `stackLayer`'s frames, for efficient lookup. */
_stackFrameSet: Set<FlamechartStackFrame>;

_internalModuleSourceToRanges: InternalModuleSourceToRanges;

_intrinsicSize: Size;

_hoveredStackFrame: FlamechartStackFrame | null = null;
Expand All @@ -85,11 +89,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 +166,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 +188,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 +282,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 +312,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
@@ -0,0 +1,16 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

export const outerErrorA = new Error();

export const moduleStartError = new Error();
export const innerError = new Error();
export const moduleStopError = new Error();

export const outerErrorB = new Error();
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

export const moduleAStartError = new Error();
export const innerErrorA = new Error();
export const moduleAStopError = new Error();

export const outerError = new Error();

export const moduleBStartError = new Error();
export const innerErrorB = new Error();
export const moduleBStopError = new Error();
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import {isInternalModule} from '../moduleFilters';

describe('isInternalModule', () => {
let map;

function createFlamechartStackFrame(scriptUrl, locationLine, locationColumn) {
return {
name: 'test',
timestamp: 0,
duration: 1,
scriptUrl,
locationLine,
locationColumn,
};
}

function createStackFrame(fileName, lineNumber, columnNumber) {
return {
columnNumber: columnNumber,
lineNumber: lineNumber,
fileName: fileName,
functionName: 'test',
source: ` at test (${fileName}:${lineNumber}:${columnNumber})`,
};
}

beforeEach(() => {
map = new Map();
map.set('foo', [
[createStackFrame('foo', 10, 0), createStackFrame('foo', 15, 100)],
]);
map.set('bar', [
[createStackFrame('bar', 10, 0), createStackFrame('bar', 15, 100)],
[createStackFrame('bar', 20, 0), createStackFrame('bar', 25, 100)],
]);
});

it('should properly identify stack frames within the provided module ranges', () => {
expect(
isInternalModule(map, createFlamechartStackFrame('foo', 10, 0)),
).toBe(true);
expect(
isInternalModule(map, createFlamechartStackFrame('foo', 12, 35)),
).toBe(true);
expect(
isInternalModule(map, createFlamechartStackFrame('foo', 15, 100)),
).toBe(true);
expect(
isInternalModule(map, createFlamechartStackFrame('bar', 12, 0)),
).toBe(true);
expect(
isInternalModule(map, createFlamechartStackFrame('bar', 22, 125)),
).toBe(true);
});

it('should properly identify stack frames outside of the provided module ranges', () => {
expect(isInternalModule(map, createFlamechartStackFrame('foo', 9, 0))).toBe(
false,
);
expect(
isInternalModule(map, createFlamechartStackFrame('foo', 15, 101)),
).toBe(false);
expect(
isInternalModule(map, createFlamechartStackFrame('bar', 17, 0)),
).toBe(false);
expect(
isInternalModule(map, createFlamechartStackFrame('baz', 12, 0)),
).toBe(false);
});
});
jstejada marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import type {
FlamechartStackFrame,
InternalModuleSourceToRanges,
} from '../../types';

import {
CHROME_WEBSTORE_EXTENSION_ID,
INTERNAL_EXTENSION_ID,
LOCAL_EXTENSION_ID,
} from 'react-devtools-shared/src/constants';

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

if (scriptUrl == null || locationColumn == null || locationLine == null) {
// This could indicate a browser-internal API like performance.mark().
return false;
jstejada marked this conversation as resolved.
Show resolved Hide resolved
}

// 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) ||
jstejada marked this conversation as resolved.
Show resolved Hide resolved
scriptUrl.includes(INTERNAL_EXTENSION_ID) ||
scriptUrl.includes(LOCAL_EXTENSION_ID)
// Unfortunately this won't get everything, like relatively loaded chunks or Web Worker files.
) {
return true;
}

// Filter out React internal packages.
const ranges = internalModuleSourceToRanges.get(scriptUrl);
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) {
jstejada marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
}
}

return false;
}
Loading