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

Reimplement the Canvas transformation functions #724

Merged
merged 1 commit into from Jun 20, 2023

Conversation

m-sasha
Copy link
Contributor

@m-sasha m-sasha commented Jun 19, 2023

via the corresponding Skia functions directly, rather than by concatenating with a transformation matrix.

This appears to be significantly faster. Specifically in Compose, the canvas is translated for every node twice (by its position, and then back). Changing the translate function to perform a direct call to Skia transform makes drawing simple nodes much faster.

@m-sasha m-sasha requested a review from igordmn June 19, 2023 09:30
@m-sasha m-sasha force-pushed the m-sasha/direct-canvas-transformations branch 2 times, most recently from 61f7f23 to 8a83c49 Compare June 19, 2023 10:36
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Could you please post your benchmark / measurements for both JVM and native?

@m-sasha
Copy link
Contributor Author

m-sasha commented Jun 19, 2023

Could you please post your benchmark / measurements for both JVM and native?

The improvement I saw was in the "Color Sparks" benchmark from the Fleet team. It's just a huge amount of small nodes that redraw each frame.

I don't think it's particularly important to "prove" the performance improvement in this case because this particular change doesn't really have a chance to make it worse. Plus, it makes sense regardless of performance.

@m-sasha m-sasha force-pushed the m-sasha/direct-canvas-transformations branch from a9c01ca to 19c4114 Compare June 19, 2023 11:46
@MatkovIvan
Copy link
Member

MatkovIvan commented Jun 19, 2023

this particular change doesn't really have a chance to make it worse

It does. Native calls might be expensive 1 2

Compared to a pure Java method call, calling a user-written native method usually has a significant overhead. The reasons for this are as much to do with optimisations that the JVM can't make compared to regular Java methods:

  • the JVM can't inline the native method;
  • the JVM doesn't know enough about the method to make optimisations that it could make when compiling a regular Java method (for example, it has to assume that all of the parameters passed in are always used);
  • the JVM can't make other optimisations that it could make if it were dynamically compiling the code (e.g. compiling a constant parameter is a constant operand to a machine instruction rather than placing it on the stack and reading it off again);
    in order to make the call into the DLL or library, the JVM may have to perform extra work, such as rearranging items on the stack.

image

@m-sasha m-sasha force-pushed the m-sasha/direct-canvas-transformations branch from 19c4114 to 288c143 Compare June 19, 2023 12:15
@m-sasha
Copy link
Contributor Author

m-sasha commented Jun 19, 2023

@MatkovIvan Indeed, but this PR replaces a call to toInterop + a native call with just one native call.

@m-sasha m-sasha force-pushed the m-sasha/direct-canvas-transformations branch 2 times, most recently from 8a83c49 to 1f3f353 Compare June 19, 2023 12:54
… Skia functions, rather than by concatenating with a transformation matrix.
@m-sasha m-sasha force-pushed the m-sasha/direct-canvas-transformations branch from 1f3f353 to b8562d8 Compare June 20, 2023 13:55
@m-sasha m-sasha merged commit 1c90124 into master Jun 20, 2023
5 checks passed
@m-sasha m-sasha deleted the m-sasha/direct-canvas-transformations branch June 20, 2023 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants