Draw power tracks faster by skipping samples that would be displayed on the same pixel#4636
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4636 +/- ##
==========================================
+ Coverage 88.47% 88.61% +0.13%
==========================================
Files 295 294 -1
Lines 26213 26115 -98
Branches 7068 7038 -30
==========================================
- Hits 23193 23142 -51
+ Misses 2809 2768 -41
+ Partials 211 205 -6
☔ View full report in Codecov by Sentry. |
|
I followed Julien's idea of showing only the min and max values. Here is a profile of repeatedly loading the 785k samples power profile: https://share.firefox.dev/3OGL1ef The work related to showing the canvases of power tracks is down to about 100ms, and the chart looks exactly like after #4634. |
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks, I think this is a very nice performance improvement!
I added a comment to explain the algorithm in the code comment for a bit though, as it wasn't as easy to figure out for me. Other than that it looks great!
I think it needs at least a test to exercise this code path. But again, I think it's not going to be easy to add one. It should be possible to change the timing values here to make them overlap probably:
But it's okay if you prefer to defer that to a follow-up. It could be good to file an issue for it. Let me know what you think!
| // a stroke applied to it. | ||
| x = (samples.time[i] - rangeStart) * millisecondWidth; | ||
| const getX = (i) => | ||
| Math.round((samples.time[i] - rangeStart) * millisecondWidth); |
There was a problem hiding this comment.
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.
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.
| // - 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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for inproving the comment!
…on the same pixel.
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for testing this behavior! Looks great to me!
| // - 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. |
There was a problem hiding this comment.
Thanks for inproving the comment!
2 participants
When loading a large power profile, a lot of time is spent drawing the canvas of the power tracks. I think we can speed this up by avoiding the call to the canvas APIs when we would be drawing multiple times in a row at the same position.
Here are the profiles I've used to test:
And profiles of the profiler loading them multiple times first without and then with the patch:
The shapes of the charts are almost the same with and without the patch (some lines are a little bit thicker without the patch). We could speed things up further if we accepted altering the shape a bit more.