-
Notifications
You must be signed in to change notification settings - Fork 49k
Scheduling Profiler marks should include thrown Errors #22419
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,15 +136,23 @@ export class ReactMeasuresView extends View { | |
// Commit phase rects are overlapped by layout and passive rects, | ||
// and it looks bad if text flows underneath/behind these overlayed rects. | ||
if (nextMeasure != null) { | ||
textRect = { | ||
...measureRect, | ||
size: { | ||
width: | ||
timestampToPosition(nextMeasure.timestamp, scaleFactor, frame) - | ||
x, | ||
height: REACT_MEASURE_HEIGHT, | ||
}, | ||
}; | ||
// This clipping shouldn't apply for measures that don't overlap though, | ||
// like passive effects that are processed after a delay, | ||
// or if there are now layout or passive effects and the next measure is render or idle. | ||
if (nextMeasure.timestamp < measure.timestamp + measure.duration) { | ||
textRect = { | ||
...measureRect, | ||
size: { | ||
width: | ||
timestampToPosition( | ||
nextMeasure.timestamp, | ||
scaleFactor, | ||
frame, | ||
) - x, | ||
height: REACT_MEASURE_HEIGHT, | ||
}, | ||
}; | ||
} | ||
Comment on lines
+139
to
+155
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice this overflow bug while I was testing profiling data sets, so I fixed it too. It's unrelated to the main change. |
||
} | ||
break; | ||
case 'render-idle': | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,241 @@ | ||
/** | ||
* 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 {ThrownError, ReactProfilerData} from '../types'; | ||
import type { | ||
Interaction, | ||
MouseMoveInteraction, | ||
Rect, | ||
Size, | ||
ViewRefs, | ||
} from '../view-base'; | ||
|
||
import { | ||
positioningScaleFactor, | ||
timestampToPosition, | ||
positionToTimestamp, | ||
widthToDuration, | ||
} from './utils/positioning'; | ||
import { | ||
View, | ||
Surface, | ||
rectContainsPoint, | ||
rectIntersectsRect, | ||
intersectionOfRects, | ||
} from '../view-base'; | ||
import { | ||
COLORS, | ||
TOP_ROW_PADDING, | ||
REACT_EVENT_DIAMETER, | ||
BORDER_SIZE, | ||
} from './constants'; | ||
|
||
const EVENT_ROW_HEIGHT_FIXED = | ||
TOP_ROW_PADDING + REACT_EVENT_DIAMETER + TOP_ROW_PADDING; | ||
|
||
export class ThrownErrorsView extends View { | ||
_profilerData: ReactProfilerData; | ||
_intrinsicSize: Size; | ||
_hoveredEvent: ThrownError | null = null; | ||
onHover: ((event: ThrownError | null) => void) | null = null; | ||
|
||
constructor(surface: Surface, frame: Rect, profilerData: ReactProfilerData) { | ||
super(surface, frame); | ||
this._profilerData = profilerData; | ||
|
||
this._intrinsicSize = { | ||
width: this._profilerData.duration, | ||
height: EVENT_ROW_HEIGHT_FIXED, | ||
}; | ||
} | ||
|
||
desiredSize() { | ||
return this._intrinsicSize; | ||
} | ||
|
||
setHoveredEvent(hoveredEvent: ThrownError | null) { | ||
if (this._hoveredEvent === hoveredEvent) { | ||
return; | ||
} | ||
this._hoveredEvent = hoveredEvent; | ||
this.setNeedsDisplay(); | ||
} | ||
|
||
/** | ||
* Draw a single `ThrownError` as a circle in the canvas. | ||
*/ | ||
_drawSingleThrownError( | ||
context: CanvasRenderingContext2D, | ||
rect: Rect, | ||
thrownError: ThrownError, | ||
baseY: number, | ||
scaleFactor: number, | ||
showHoverHighlight: boolean, | ||
) { | ||
const {frame} = this; | ||
const {timestamp} = thrownError; | ||
|
||
const x = timestampToPosition(timestamp, scaleFactor, frame); | ||
const radius = REACT_EVENT_DIAMETER / 2; | ||
const eventRect: Rect = { | ||
origin: { | ||
x: x - radius, | ||
y: baseY, | ||
}, | ||
size: {width: REACT_EVENT_DIAMETER, height: REACT_EVENT_DIAMETER}, | ||
}; | ||
if (!rectIntersectsRect(eventRect, rect)) { | ||
return; // Not in view | ||
} | ||
|
||
const fillStyle = showHoverHighlight | ||
? COLORS.REACT_THROWN_ERROR_HOVER | ||
: COLORS.REACT_THROWN_ERROR; | ||
|
||
const y = eventRect.origin.y + radius; | ||
|
||
context.beginPath(); | ||
context.fillStyle = fillStyle; | ||
context.arc(x, y, radius, 0, 2 * Math.PI); | ||
context.fill(); | ||
} | ||
|
||
draw(context: CanvasRenderingContext2D) { | ||
const { | ||
frame, | ||
_profilerData: {thrownErrors}, | ||
_hoveredEvent, | ||
visibleArea, | ||
} = this; | ||
|
||
context.fillStyle = COLORS.BACKGROUND; | ||
context.fillRect( | ||
visibleArea.origin.x, | ||
visibleArea.origin.y, | ||
visibleArea.size.width, | ||
visibleArea.size.height, | ||
); | ||
|
||
// Draw events | ||
const baseY = frame.origin.y + TOP_ROW_PADDING; | ||
const scaleFactor = positioningScaleFactor( | ||
this._intrinsicSize.width, | ||
frame, | ||
); | ||
|
||
const highlightedEvents: ThrownError[] = []; | ||
|
||
thrownErrors.forEach(thrownError => { | ||
if (thrownError === _hoveredEvent) { | ||
highlightedEvents.push(thrownError); | ||
return; | ||
} | ||
this._drawSingleThrownError( | ||
context, | ||
visibleArea, | ||
thrownError, | ||
baseY, | ||
scaleFactor, | ||
false, | ||
); | ||
}); | ||
|
||
// Draw the highlighted items on top so they stand out. | ||
// This is helpful if there are multiple (overlapping) items close to each other. | ||
highlightedEvents.forEach(thrownError => { | ||
this._drawSingleThrownError( | ||
context, | ||
visibleArea, | ||
thrownError, | ||
baseY, | ||
scaleFactor, | ||
true, | ||
); | ||
}); | ||
|
||
// Render bottom borders. | ||
// Propose border rect, check if intersects with `rect`, draw intersection. | ||
const borderFrame: Rect = { | ||
origin: { | ||
x: frame.origin.x, | ||
y: frame.origin.y + EVENT_ROW_HEIGHT_FIXED - BORDER_SIZE, | ||
}, | ||
size: { | ||
width: frame.size.width, | ||
height: BORDER_SIZE, | ||
}, | ||
}; | ||
if (rectIntersectsRect(borderFrame, visibleArea)) { | ||
const borderDrawableRect = intersectionOfRects(borderFrame, visibleArea); | ||
context.fillStyle = COLORS.REACT_WORK_BORDER; | ||
context.fillRect( | ||
borderDrawableRect.origin.x, | ||
borderDrawableRect.origin.y, | ||
borderDrawableRect.size.width, | ||
borderDrawableRect.size.height, | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
_handleMouseMove(interaction: MouseMoveInteraction, viewRefs: ViewRefs) { | ||
const {frame, onHover, visibleArea} = this; | ||
if (!onHover) { | ||
return; | ||
} | ||
|
||
const {location} = interaction.payload; | ||
if (!rectContainsPoint(location, visibleArea)) { | ||
onHover(null); | ||
return; | ||
} | ||
|
||
const { | ||
_profilerData: {thrownErrors}, | ||
} = this; | ||
const scaleFactor = positioningScaleFactor( | ||
this._intrinsicSize.width, | ||
frame, | ||
); | ||
const hoverTimestamp = positionToTimestamp(location.x, scaleFactor, frame); | ||
const eventTimestampAllowance = widthToDuration( | ||
REACT_EVENT_DIAMETER / 2, | ||
scaleFactor, | ||
); | ||
|
||
// Because data ranges may overlap, we want to find the last intersecting item. | ||
// This will always be the one on "top" (the one the user is hovering over). | ||
for (let index = thrownErrors.length - 1; index >= 0; index--) { | ||
const event = thrownErrors[index]; | ||
const {timestamp} = event; | ||
|
||
if ( | ||
timestamp - eventTimestampAllowance <= hoverTimestamp && | ||
hoverTimestamp <= timestamp + eventTimestampAllowance | ||
) { | ||
this.currentCursor = 'context-menu'; | ||
viewRefs.hoveredView = this; | ||
onHover(event); | ||
return; | ||
} | ||
} | ||
|
||
onHover(null); | ||
} | ||
|
||
handleInteraction(interaction: Interaction, viewRefs: ViewRefs) { | ||
switch (interaction.type) { | ||
case 'mousemove': | ||
this._handleMouseMove(interaction, viewRefs); | ||
break; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,9 +177,11 @@ export const THEME_STYLES: {[style: Theme | DisplayDensity]: any} = { | |
'--color-scheduling-profiler-react-suspense-resolved-hover': '#89d281', | ||
'--color-scheduling-profiler-react-suspense-unresolved': '#c9cacd', | ||
'--color-scheduling-profiler-react-suspense-unresolved-hover': '#93959a', | ||
'--color-scheduling-profiler-thrown-error': '#ee1638', | ||
'--color-scheduling-profiler-thrown-error-hover': '#da1030', | ||
'--color-scheduling-profiler-text-color': '#000000', | ||
'--color-scheduling-profiler-text-dim-color': '#ccc', | ||
'--color-scheduling-profiler-react-work-border': '#ffffff', | ||
'--color-scheduling-profiler-react-work-border': '#eeeeee', | ||
'--color-search-match': 'yellow', | ||
'--color-search-match-current': '#f7923b', | ||
'--color-selected-tree-highlight-active': 'rgba(0, 136, 250, 0.1)', | ||
|
@@ -316,9 +318,11 @@ export const THEME_STYLES: {[style: Theme | DisplayDensity]: any} = { | |
'--color-scheduling-profiler-react-suspense-resolved-hover': '#89d281', | ||
'--color-scheduling-profiler-react-suspense-unresolved': '#c9cacd', | ||
'--color-scheduling-profiler-react-suspense-unresolved-hover': '#93959a', | ||
'--color-scheduling-profiler-thrown-error': '#fb3655', | ||
'--color-scheduling-profiler-thrown-error-hover': '#f82042', | ||
'--color-scheduling-profiler-text-color': '#282c34', | ||
'--color-scheduling-profiler-text-dim-color': '#555b66', | ||
'--color-scheduling-profiler-react-work-border': '#ffffff', | ||
'--color-scheduling-profiler-react-work-border': '#3d424a', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These color values were broken before. Noticed while implementing this so I fixed them too. |
||
'--color-search-match': 'yellow', | ||
'--color-search-match-current': '#f7923b', | ||
'--color-selected-tree-highlight-active': 'rgba(23, 143, 185, 0.15)', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the repetition in this file was driving me crazy so I cleaned it up a bit with this change.