Skip to content

Change Flamegraph handling of narrow boxes (snap to multiples of 2 device pixels) #4601

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.

Sign up for GitHub

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

Merged
mstange merged 3 commits into from
May 9, 2023
Merged

Change Flamegraph handling of narrow boxes (snap to multiples of 2 device pixels) #4601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f2e359
Supply cssToDeviceScale to drawCanvas.
mstange May 1, 2023
8c44ab9
Cache TextMeasurer for stack chart / js tracer.
mstange May 1, 2023
2bf992e
Draw Flamegraph in device pixels.
mstange May 1, 2023
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
139 changes: 115 additions & 24 deletions src/components/flame-graph/Canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
Thread,
CategoryList,
CssPixels,
DevicePixels,
Milliseconds,
CallNodeInfo,
IndexIntoCallNodeTable,
Expand All @@ -43,6 +44,11 @@ import type {
IndexIntoFlameGraphTiming,
} from 'firefox-profiler/profile-logic/flame-graph';

import type {
ChartCanvasScale,
ChartCanvasHoverInfo,
} from '../shared/chart/Canvas';

import type { CallTree } from 'firefox-profiler/profile-logic/call-tree';

export type OwnProps = {|
Expand Down Expand Up @@ -90,9 +96,32 @@ import './Canvas.css';
const ROW_HEIGHT = 16;
const TEXT_OFFSET_START = 3;
const TEXT_OFFSET_TOP = 11;
const FONT_SIZE = 10;

/**
* Round the given value to integers, consistently rounding x.5 towards positive infinity.
* This is different from Math.round: Math.round rounds 0.5 to the right (to 1), and -0.5
* to the left (to -1).
* snap should be preferred over Math.round for rounding coordinates which might
* be negative, so that there is no discontinuity when a box moves past zero.
*/
function snap(floatDeviceValue: DevicePixels): DevicePixels {
return Math.floor(floatDeviceValue + 0.5);
}

/**
* Round the given value to a multiple of `integerFactor`.
*/
function snapValueToMultipleOf(
floatDeviceValue: DevicePixels,
integerFactor: number
): DevicePixels {
return snap(floatDeviceValue / integerFactor) * integerFactor;
}

class FlameGraphCanvasImpl extends React.PureComponent<Props> {
_textMeasurement: null | TextMeasurement;
_textMeasurementCssToDeviceScale: number = 1;

componentDidUpdate(prevProps) {
// If the stack depth changes (say, when changing the time range
Expand Down Expand Up @@ -149,7 +178,8 @@ class FlameGraphCanvasImpl extends React.PureComponent<Props> {

_drawCanvas = (
ctx: CanvasRenderingContext2D,
hoveredItem: HoveredStackTiming | null
scale: ChartCanvasScale,
hoverInfo: ChartCanvasHoverInfo<HoveredStackTiming>
) => {
const {
thread,
Expand All @@ -168,23 +198,51 @@ class FlameGraphCanvasImpl extends React.PureComponent<Props> {
},
} = this.props;

const { hoveredItem } = hoverInfo;

const { cssToDeviceScale, cssToUserScale } = scale;
if (cssToDeviceScale !== cssToUserScale) {
throw new Error(
'FlameGraphCanvasImpl sets scaleCtxToCssPixels={false}, so canvas user space units should be equal to device pixels.'
);
}

const deviceContainerWidth = containerWidth * cssToDeviceScale;
const deviceContainerHeight = containerHeight * cssToDeviceScale;

// Set the font before creating the text renderer. The font property resets
// automatically whenever the canvas size is changed, so we set it on every
// call.
ctx.font = `${FONT_SIZE * cssToDeviceScale}px sans-serif`;

// Ensure the text measurement tool is created, since this is the first time
// this class has access to a ctx.
if (!this._textMeasurement) {
// this class has access to a ctx. We also need to recreate it when the scale
// changes because we are working with device coordinates.
if (
!this._textMeasurement ||
this._textMeasurementCssToDeviceScale !== cssToDeviceScale
) {
this._textMeasurement = new TextMeasurement(ctx);
this._textMeasurementCssToDeviceScale = cssToDeviceScale;
}

const textMeasurement = this._textMeasurement;

const fastFillStyle = new FastFillStyle(ctx);
const deviceHorizontalPadding: DevicePixels = Math.round(
TEXT_OFFSET_START * cssToDeviceScale
);

fastFillStyle.set('#ffffff');
ctx.fillRect(0, 0, containerWidth, containerHeight);
ctx.fillRect(0, 0, deviceContainerWidth, deviceContainerHeight);

const startDepth = Math.floor(
maxStackDepth - viewportBottom / stackFrameHeight
);
const endDepth = Math.ceil(maxStackDepth - viewportTop / stackFrameHeight);

// Only draw the stack frames that are vertically within view.
// The graph is drawn from bottom to top, in order of increasing depth.
for (let depth = startDepth; depth < endDepth; depth++) {
// Get the timing information for a row of stack frames.
const stackTiming = flameGraphTiming[depth];
Expand All @@ -193,19 +251,45 @@ class FlameGraphCanvasImpl extends React.PureComponent<Props> {
continue;
}

const cssRowTop: CssPixels =
(maxStackDepth - depth - 1) * ROW_HEIGHT - viewportTop;
const cssRowBottom: CssPixels =
(maxStackDepth - depth) * ROW_HEIGHT - viewportTop;
const deviceRowTop: DevicePixels = snap(cssRowTop * cssToDeviceScale);
const deviceRowBottom: DevicePixels =
snap(cssRowBottom * cssToDeviceScale) - 1;
const deviceRowHeight: DevicePixels = deviceRowBottom - deviceRowTop;

const deviceTextTop =
deviceRowTop + snap(TEXT_OFFSET_TOP * cssToDeviceScale);

for (let i = 0; i < stackTiming.length; i++) {
const startTime = stackTiming.start[i];
const endTime = stackTiming.end[i];
// For each box, snap the left and right edges to the nearest multiple
// of two device pixels. If both edges snap to the same value, the box
// becomes empty and is not drawn.
Comment on lines +267 to +269

@julienw julienw May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure of this. I think we should snap to at least 1px if there's a box, but do it just for one such small box on this px (if there are more than one box here), and only if there's no bigger box at this location.
This would be similar to (but not completely the same) what we do in the marker chart.
What do you think?

@julienw julienw May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Currently, when selecting a preview while viewing the flame graph on your deploy preview, we see "peaks" showing up and then disappearing very often. I don't know if this is the reason, but I'd expect the peaks to still show up even if they have a small width. It would be great if the view would stay mostly stable.

@mstange mstange May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's true, both of these aspects are "cons" to the "just snap" approach. But I think they're outweighed by the "pros": The algorithm is simple and fast, and it never produces incorrect nesting: If box A is rendered below box B (in the same pixel column), you know that A is the caller of B. It seems hard to preserve peaks while also preserving correct nesting.

For what it's worth, Chrome's devtools performance panel also has flickering peaks as you zoom in and out.

//
// Boxes which remain are at least two device pixels wide. We create a
// translucent gap the end of each box by shifting the right edge to the
// left by 0.8 device pixels, so that this gap pixel column is filled to
// 20%.

const boxLeftFraction = stackTiming.start[i];
const boxRightFraction = stackTiming.end[i];
const deviceBoxLeftUnsnapped = boxLeftFraction * deviceContainerWidth;
const deviceBoxRightUnsnapped = boxRightFraction * deviceContainerWidth;

const deviceBoxLeft: DevicePixels = snapValueToMultipleOf(
deviceBoxLeftUnsnapped,
2
);
const deviceBoxRight: DevicePixels =
snapValueToMultipleOf(deviceBoxRightUnsnapped, 2) - 0.8;

const w: CssPixels = (endTime - startTime) * containerWidth;
if (w < 2) {
// Skip sending draw calls for sufficiently small boxes.
const deviceBoxWidth: DevicePixels = deviceBoxRight - deviceBoxLeft;
if (deviceBoxWidth <= 0) {
// Skip drawing boxes which snapped away to nothing.
continue;
}
const x: CssPixels = startTime * containerWidth;
const y: CssPixels =
(maxStackDepth - depth - 1) * ROW_HEIGHT - viewportTop;
const h: CssPixels = ROW_HEIGHT - 1;

const callNodeIndex = stackTiming.callNode[i];
const isSelected = selectedCallNodeIndex === callNodeIndex;
Expand All @@ -227,25 +311,32 @@ class FlameGraphCanvasImpl extends React.PureComponent<Props> {
: colorStyles.unselectedFillStyle;

fastFillStyle.set(background);
// Draw rect at an offset to ensure spacing between blocks.
ctx.fillRect(x + 1, y, w - 1, h);

// TODO - L10N RTL.
// Constrain the x coordinate to the leftmost area.
const x2: CssPixels = Math.max(x, 0) + TEXT_OFFSET_START;
const w2: CssPixels = Math.max(0, w - (x2 - x));
if (w2 > textMeasurement.minWidth) {
ctx.fillRect(
deviceBoxLeft,
deviceRowTop,
deviceBoxWidth,
deviceRowHeight
);

const deviceTextLeft: DevicePixels =
deviceBoxLeft + deviceHorizontalPadding;
const deviceTextWidth: DevicePixels = deviceBoxRight - deviceTextLeft;
if (deviceTextWidth > textMeasurement.minWidth) {
const funcIndex = callNodeTable.func[callNodeIndex];
const funcName = thread.stringTable.getString(
thread.funcTable.name[funcIndex]
);
const fittedText = textMeasurement.getFittedText(funcName, w2);
const fittedText = textMeasurement.getFittedText(
funcName,
deviceTextWidth
);
if (fittedText) {
const foreground = isHighlighted
? colorStyles.selectedTextColor
: '#000';
fastFillStyle.set(foreground);
ctx.fillText(fittedText, x2, y + TEXT_OFFSET_TOP);
// TODO - L10N RTL.
ctx.fillText(fittedText, deviceTextLeft, deviceTextTop);
}
}
}
Expand Down Expand Up @@ -413,7 +504,7 @@ class FlameGraphCanvasImpl extends React.PureComponent<Props> {
containerWidth={containerWidth}
containerHeight={containerHeight}
isDragging={isDragging}
scaleCtxToCssPixels={true}
scaleCtxToCssPixels={false}
onDoubleClickItem={this._onDoubleClick}
getHoveredItemInfo={this._getHoveredStackInfo}
drawCanvas={this._drawCanvas}
Expand Down
59 changes: 43 additions & 16 deletions src/components/js-tracer/Canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ import type {
JsTracerTiming,
} from 'firefox-profiler/types';

import type {
ChartCanvasScale,
ChartCanvasHoverInfo,
} from '../shared/chart/Canvas';

import type { WrapFunctionInDispatch } from 'firefox-profiler/utils/connect';

type OwnProps = {|
Expand Down Expand Up @@ -92,14 +97,17 @@ class JsTracerCanvasImpl extends React.PureComponent<Props, State> {
state = {
hasFirstDraw: false,
};
_textMeasurement: null | TextMeasurement;
_textMeasurementCssToDeviceScale: number = 1;

/**
* This method is called by the ChartCanvas component whenever the canvas needs to
* be painted.
*/
drawCanvas = (
ctx: CanvasRenderingContext2D,
hoveredItem: IndexIntoJsTracerEvents | null
scale: ChartCanvasScale,
hoverInfo: ChartCanvasHoverInfo<IndexIntoJsTracerEvents>
) => {
const {
rowHeight,
Expand All @@ -111,15 +119,34 @@ class JsTracerCanvasImpl extends React.PureComponent<Props, State> {
containerHeight,
},
} = this.props;
const { hoveredItem } = hoverInfo;

const { devicePixelRatio } = window;
const { cssToDeviceScale, cssToUserScale } = scale;
if (cssToDeviceScale !== cssToUserScale) {
throw new Error(
'JsTracerCanvasImpl sets scaleCtxToCssPixels={false}, so canvas user space units should be equal to device pixels.'
);
}

// Set the font size before creating a text measurer.
ctx.font = `${FONT_SIZE * devicePixelRatio}px sans-serif`;
// Set the font before creating the text renderer. The font property resets
// automatically whenever the canvas size is changed, so we set it on every
// call.
ctx.font = `${FONT_SIZE * cssToDeviceScale}px sans-serif`;

// Ensure the text measurement tool is created, since this is the first time
// this class has access to a ctx. We also need to recreate it when the scale
// changes because we are working with device coordinates.
if (
!this._textMeasurement ||
this._textMeasurementCssToDeviceScale !== cssToDeviceScale
) {
this._textMeasurement = new TextMeasurement(ctx);
this._textMeasurementCssToDeviceScale = cssToDeviceScale;
}

@julienw julienw May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

optional suggestion: use memoize-one with a function taking cssToDeviceScale as parameter. This would move the logic to invalidate the text measurement tool out of this function.

@julienw julienw May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also this could possibly be handled directly by the shared Canvas code, passed to drawCanvas with the other properties. Because it looks like that the same code is repeated in all these charts.

@mstange mstange May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, these suggestions would improve things! I'll leave them for follow-ups though.


const renderPass: RenderPass = {
ctx,
textMeasurement: new TextMeasurement(ctx),
textMeasurement: this._textMeasurement,
fastFillStyle: new FastFillStyle(ctx),
// Define a start and end row, so that we only draw the events
// that are vertically within view.
Expand All @@ -130,19 +157,19 @@ class JsTracerCanvasImpl extends React.PureComponent<Props, State> {
),
devicePixels: {
// Convert many of the common values provided by the Props into DevicePixels.
containerWidth: containerWidth * devicePixelRatio,
containerWidth: containerWidth * cssToDeviceScale,
innerContainerWidth:
(containerWidth - TIMELINE_MARGIN_LEFT - TIMELINE_MARGIN_RIGHT) *
devicePixelRatio,
containerHeight: containerHeight * devicePixelRatio,
textOffsetStart: TEXT_OFFSET_START * devicePixelRatio,
textOffsetTop: TEXT_OFFSET_TOP * devicePixelRatio,
rowHeight: rowHeight * devicePixelRatio,
viewportTop: viewportTop * devicePixelRatio,
timelineMarginLeft: TIMELINE_MARGIN_LEFT * devicePixelRatio,
timelineMarginRight: TIMELINE_MARGIN_RIGHT * devicePixelRatio,
oneCssPixel: devicePixelRatio,
rowLabelOffsetLeft: ROW_LABEL_OFFSET_LEFT * devicePixelRatio,
cssToDeviceScale,
containerHeight: containerHeight * cssToDeviceScale,
textOffsetStart: TEXT_OFFSET_START * cssToDeviceScale,
textOffsetTop: TEXT_OFFSET_TOP * cssToDeviceScale,
rowHeight: rowHeight * cssToDeviceScale,
viewportTop: viewportTop * cssToDeviceScale,
timelineMarginLeft: TIMELINE_MARGIN_LEFT * cssToDeviceScale,
timelineMarginRight: TIMELINE_MARGIN_RIGHT * cssToDeviceScale,
oneCssPixel: cssToDeviceScale,
rowLabelOffsetLeft: ROW_LABEL_OFFSET_LEFT * cssToDeviceScale,
},
};

Expand Down
23 changes: 20 additions & 3 deletions src/components/marker-chart/Canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ import type {
} from 'firefox-profiler/types';
import { getStartEndRangeForMarker } from 'firefox-profiler/utils';

import type {
ChartCanvasScale,
ChartCanvasHoverInfo,
} from '../shared/chart/Canvas';

import type { WrapFunctionInDispatch } from 'firefox-profiler/utils/connect';

type MarkerDrawingInformation = {|
Expand Down Expand Up @@ -99,9 +104,8 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {

drawCanvas = (
ctx: CanvasRenderingContext2D,
hoveredItems: HoveredMarkerChartItems | null,
prevHoveredItems: HoveredMarkerChartItems | null,
isHoveredOnlyDifferent: boolean
scale: ChartCanvasScale,
hoverInfo: ChartCanvasHoverInfo<HoveredMarkerChartItems>
) => {
const {
rowHeight,
Expand All @@ -120,6 +124,12 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
let prevHoveredMarker = null;
let prevHoveredLabel = null;

const {
hoveredItem: hoveredItems,
prevHoveredItem: prevHoveredItems,
isHoveredOnlyDifferent,
} = hoverInfo;

if (hoveredItems) {
hoveredMarker = hoveredItems.markerIndex;
hoveredLabel = hoveredItems.rowIndexOfLabel;
Expand All @@ -129,6 +139,13 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
prevHoveredLabel = prevHoveredItems.rowIndexOfLabel;
}

const { cssToUserScale } = scale;
if (cssToUserScale !== 1) {
throw new Error(
'StackChartCanvasImpl sets scaleCtxToCssPixels={true}, so canvas user space units should be equal to CSS pixels.'
);
}

// Convert CssPixels to Stack Depth
const startRow = Math.floor(viewportTop / rowHeight);
const endRow = Math.min(
Expand Down
Loading