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

Benchmarks. Mimic the behavior of Skiko rendering #5215

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Jan 27, 2025

  • Draw into an intermediate Picture. Without it, benchmarks showed an improvement by 8.61% between old/new layers, but in fact it was a regression by 5.63%:
image
  • Clear Canvas each frame

  • Add MavenLocal to becnhmark local versions

Release Notes

N/A

@igordmn igordmn requested a review from pjBooms January 27, 2025 02:49
@igordmn igordmn force-pushed the igor.demin/benchmark-mimic branch from 877b8f5 to 18147a6 Compare January 27, 2025 02:50
Currently, we include the time when we wait for vsync which gives us wrong results
- Draw into an intermediate Picture. Without it benchmarks showed an improvement by 10%, but in fact it was a regression by 10%
- Clear Canvas each frame
@igordmn igordmn force-pushed the igor.demin/benchmark-mimic branch from 18147a6 to 1bb85ba Compare January 27, 2025 14:54
private val pictureRecorder = PictureRecorder()

@OptIn(InternalComposeUiApi::class)
fun ComposeScene.mimicSkikoRender(surface: Surface, time: Long, width: Int, height: Int) {
Copy link
Member

Choose a reason for hiding this comment

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

imo: requires BIG TODO to keep it in sync with skiko implementation, because I'm not sure that it won't be changed there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment here

Without reusing the whole renderer from Skiko (which realistically we won't do without a good reason), we should keep it sync manually.

When we merge it, I will add a comment to Skiko:

Note, if you change the rendering logic a lot, notify about this in [this issue](new created issue), because [benchmarks project](link to this function) depends on it

* If Skiko stops using `picture`, we need to remove it here too.
*/
@OptIn(InternalComposeUiApi::class)
fun ComposeScene.mimicSkikoRender(surface: Surface, time: Long, width: Int, height: Int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rename it to reflect what it actually does , f.i renderViaPicture.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function should match Skiko’s rendering.

While it currently uses a picture, that’s an implementation detail. If Skiko stops using a picture, we should change the implementation here, without renaming.

It also clears the Canvas, which may impact benchmarking accuracy.

Ideally we should reuse some function from Skiko, but this a much more complex task.

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.

3 participants