-
Notifications
You must be signed in to change notification settings - Fork 480
Draw power tracks faster by skipping samples that would be displayed on the same pixel #4636
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 |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import type { | |
|
|
||
| import type { SizeProps } from 'firefox-profiler/components/shared/WithSize'; | ||
| import type { ConnectedProps } from 'firefox-profiler/utils/connect'; | ||
| import { timeCode } from 'firefox-profiler/utils/time-code'; | ||
|
|
||
| import './TrackPower.css'; | ||
|
|
||
|
|
@@ -129,34 +130,75 @@ class TrackPowerCanvas extends React.PureComponent<CanvasProps> { | |
| ctx.fillStyle = '#73737388'; // Grey 50 with transparency. | ||
| ctx.beginPath(); | ||
|
|
||
| // The x and y are used after the loop. | ||
| let x = 0; | ||
| let y = 0; | ||
| let firstX = 0; | ||
| for (let i = sampleStart; i < sampleEnd; i++) { | ||
| // Create a path for the top of the chart. This is the line that will have | ||
| // a stroke applied to it. | ||
| x = (samples.time[i] - rangeStart) * millisecondWidth; | ||
| const getX = (i) => | ||
| Math.round((samples.time[i] - rangeStart) * millisecondWidth); | ||
| const getPower = (i) => { | ||
| const sampleTimeDeltaInMs = | ||
| i === 0 ? interval : samples.time[i] - samples.time[i - 1]; | ||
| const unitGraphCount = | ||
| samples.count[i] / sampleTimeDeltaInMs / countRangePerMs; | ||
| return samples.count[i] / sampleTimeDeltaInMs; | ||
| }; | ||
| const getY = (rawY) => { | ||
| const unitGraphCount = rawY / countRangePerMs; | ||
| // Add on half the stroke's line width so that it won't be cut off the edge | ||
| // of the graph. | ||
| y = | ||
| return Math.round( | ||
| innerDeviceHeight - | ||
| innerDeviceHeight * unitGraphCount + | ||
| deviceLineHalfWidth; | ||
| if (i === 0) { | ||
| // This is the first iteration, only move the line, do not draw it. Also | ||
| // remember this first X, as the bottom of the graph will need to connect | ||
| // back up to it. | ||
| firstX = x; | ||
| ctx.moveTo(x, y); | ||
| } else { | ||
| innerDeviceHeight * unitGraphCount + | ||
| deviceLineHalfWidth | ||
| ); | ||
| }; | ||
|
|
||
| // The x and y are used after the loop. | ||
| const firstX = getX(sampleStart); | ||
| let x = firstX; | ||
| let y = getY(getPower(sampleStart)); | ||
|
|
||
| // For the first sample, only move the line, do not draw it. Also | ||
| // remember this first X, as the bottom of the graph will need to connect | ||
| // back up to it. | ||
| ctx.moveTo(x, y); | ||
|
|
||
| // Create a path for the top of the chart. This is the line that will have | ||
| // a stroke applied to it. | ||
| for (let i = sampleStart + 1; i < sampleEnd; i++) { | ||
| const powerValues = [getPower(i)]; | ||
| x = getX(i); | ||
| y = getY(powerValues[0]); | ||
| ctx.lineTo(x, y); | ||
|
|
||
| // If we have multiple samples to draw on the same horizontal pixel, | ||
| // we process all of them together with a max-min decimation algorithm | ||
| // to save time: | ||
| // - We draw the first and last samples to ensure the display is | ||
| // correct if there are sampling gaps. | ||
| // - For the values in between, we only draw the min and max values, | ||
| // to draw a vertical line covering all the other sample values. | ||
|
Member
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. it would be also good to mention the "min/max decimation" name for future reference and also maybe we can put the link that Julien mentioned in his comment? First I was a bit confused with the algorithm but looking at the description made it more obvious (I was confused with how many points we draw for each pixel)
Contributor
Author
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. I'm adding "with max-min decimation algorithm" to the wording of the comment. I was hoping "we only draw the min and max values, to draw a vertical line covering all the other sample values." in my comment was descriptive enough of how the algorithm worked, but I guess not. Thanks for suggesting the comment improvement.
Member
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. Thanks for inproving the comment! |
||
| while (i + 1 < sampleEnd && getX(i + 1) === x) { | ||
| powerValues.push(getPower(++i)); | ||
| } | ||
|
|
||
| // Looking for the min and max only makes sense if we have more than 2 | ||
| // samples to draw. | ||
| if (powerValues.length > 2) { | ||
| const minY = getY(Math.min(...powerValues)); | ||
| if (minY !== y) { | ||
| y = minY; | ||
| ctx.lineTo(x, y); | ||
| } | ||
| const maxY = getY(Math.max(...powerValues)); | ||
| if (maxY !== y) { | ||
| y = maxY; | ||
| ctx.lineTo(x, y); | ||
| } | ||
| } | ||
|
|
||
| const lastY = getY(powerValues[powerValues.length - 1]); | ||
| if (lastY !== y) { | ||
| y = lastY; | ||
| ctx.lineTo(x, y); | ||
| } | ||
| } | ||
|
|
||
| // The samples range ends at the time of the last sample, plus the interval. | ||
| // Draw this last bit. | ||
| ctx.lineTo(x + intervalWidth, y); | ||
|
|
@@ -192,7 +234,9 @@ class TrackPowerCanvas extends React.PureComponent<CanvasProps> { | |
|
|
||
| const canvas = this._canvas; | ||
| if (canvas) { | ||
| this.drawCanvas(canvas); | ||
| timeCode('TrackPowerCanvas render', () => { | ||
| this.drawCanvas(canvas); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
So differently, we are rounding the numbers with this patch. I guess this is going to change the shape of the graph a bit right? But otherwise it's pretty much impossible to implement this without rounding, and it looks like the shape doesn't change that much, so looks good to me.
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.
Even when switching back and forth between the same profile on profiler.firefox.com and on my local version with the patch applied, I couldn't see any difference in the shape of the graph. The deviceWidth is
width * devicePixelRatio, so any integer x value here is already a half pixel on a hidpi screen. I think that's the reason why there's no visible difference.