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

Basic Chart/Point Drawing Optimizations #1009

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

jeffpamer
Copy link
Contributor

@jeffpamer jeffpamer commented Nov 16, 2023

This PR does a couple things that I feel like are low-hanging fruit optimizations, and pretty "safe", i.e. shouldn't present any noticeable change relative to the current functionality. I'm interested to see after merging these in with @AaronPlave's zoom work what our baseline performance is, and then see where we need to go from there.

  • Render the data points to an offscreen canvas and then copy them as image data as needed. This should be quite faster than redrawing new arc paths every time.
  • Splits rendering the line and points into two separate frames. This has two advantages:
    1. Rendering the lines and points across separate frames means we give the main thread a chance to unblock in-between
    2. If we get several immediate requests to redraw the line, we can cancel the existing requests to draw the points and only draw them once at the end. Effectively debouncing them. If we want I think we could be even more aggressive about this, i.e. delay drawing the points until after 50ms pass from the original draw request.

@AaronPlave
Copy link
Contributor

AaronPlave commented Nov 16, 2023

I get this when I load up a particular view - think it might be an issue with points with 0 size?
image

@AaronPlave
Copy link
Contributor

Also I'm seeing this shape for the points on my machine
image

@duranb
Copy link
Collaborator

duranb commented Nov 16, 2023

Also I'm seeing this shape for the points on my machine
image

Interesting...I'm not seeing that shape. Do you get that for a specific resource's plot or for all line charts? Also are you on a new m2 mac or intel?

@duranb
Copy link
Collaborator

duranb commented Nov 16, 2023

@jeffpamer do you think we should wait until @AaronPlave's zooming work is done so that we can fully test this?

@jeffpamer
Copy link
Contributor Author

I get this when I load up a particular view - think it might be an issue with points with 0 size?

Oh interesting, I didn't realize points could be zero size, I'll put in something to handle that ... likely just skip the point rendering entirely?

Do you think we should wait until @AaronPlave's zooming work is done so that we can fully test this?

I was thinking it would be good to get this merged in first and then Aaron could just pull it into his branch to test it out. But looks like there's a couple issues to sort out! I'll look into them but not sure if I'll have enough time to solve today.

@jeffpamer
Copy link
Contributor Author

I might try to carve out some time while traveling next week to look into this more, but otherwise I'll just need to pick it up the week after!

@jeffpamer jeffpamer force-pushed the feature/chart-optimizations branch from 92791d5 to 99db6fd Compare December 6, 2023 23:08
@jeffpamer
Copy link
Contributor Author

jeffpamer commented Dec 6, 2023

@AaronPlave @duranb sorry for the delay here but I think this should be patched up and ready to go, would you mind taking another look Aaron?

The gist of the problem is that you typically handle high resolution rendering by scaling up your canvas size and context scale factor by devicePixelRatio to draw in high resolution and then scale the canvas down to it's expected pixel size using CSS to get the expected result. But OffscreenCanvas doesn't support sizing by CSS, so instead what we need to do is scale up the canvas and draw at 2x size like usual, but then where we copy the image data over to the onscreen canvas, explicitly specify the size we want to draw each point in standard screen pixels.

A good little "a ha" moment for working with OffscreenCanvas

Copy link
Contributor

@AaronPlave AaronPlave left a comment

Choose a reason for hiding this comment

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

LGTM. Does look like the drawImage performance + deferred rendering of these points does result in at least slightly better performance for a large collection of resource plots with many points.

@jeffpamer jeffpamer self-assigned this Dec 7, 2023
@jeffpamer
Copy link
Contributor Author

LGTM. Does look like the drawImage performance + deferred rendering of these points does result in at least slightly better performance for a large collection of resource plots with many points.

Cool good to hear, my hope would be the "perceived performance" would be the biggest benefit since the deferred rendering would give the main thread a chance to flush. We can also look into maybe tweaking exactly how long to delay the point rendering ...

@jeffpamer jeffpamer merged commit 5f94ae3 into develop Dec 7, 2023
4 checks passed
@jeffpamer jeffpamer deleted the feature/chart-optimizations branch December 7, 2023 20:06
JosephVolosin pushed a commit that referenced this pull request Aug 20, 2024
* Render points using offscreen canvas, split line and point drawing into two separate frames

* account for devicepixelratio scaling, don't attempt to draw 0 radius points
JosephVolosin pushed a commit that referenced this pull request Oct 21, 2024
* Render points using offscreen canvas, split line and point drawing into two separate frames

* account for devicepixelratio scaling, don't attempt to draw 0 radius points
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants