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

Swap start/finish order for rendering #630

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ryantrem
Copy link
Member

@ryantrem ryantrem commented Jan 4, 2024

In this change, the order of start/finish for rendering (Graphics::Device and Graphics::DeviceUpdate) is swapped (similar to what was already done in the BN Playground app). This is more efficient as it results in less blocked time on the main thread, and should also allow the JS thread to start its per-frame work earlier. Without these changes, there can be more "fighting" for CPU time between BN and platform UI. I tested these changes by adding a large React Native FlatList into the main screen of the Playground app, and testing how scrolling the FlatList while interacting with the scene affected performance. Just looking at the frame rate, it seems like I get about a 10% improvement in performance (both Android and iOS). I expect we'd get better gains on Android with future work moving the rendering off the main thread entirely.

Testing
I tested pretty thoroughly with the Playground app on Android and iOS. I wasn't able to get a Windows build working (haven't tried in a long time), so I haven't tested on Windows (could use some help here 🙏🏻).

Fixes #343

@CedricGuillemet
Copy link
Contributor

I'm still able to test Windows build. I'll check it this week.

@CedricGuillemet
Copy link
Contributor

I guess it fixes #343

@matthargett
Copy link

Can this be rebased and retested?

@ryantrem
Copy link
Member Author

@CedricGuillemet you did test this a while back on Windows and something wasn't working, correct? Do you recall what the issue was? Can you add some notes to this PR discussion?

@CedricGuillemet
Copy link
Contributor

@CedricGuillemet you did test this a while back on Windows and something wasn't working, correct? Do you recall what the issue was? Can you add some notes to this PR discussion?

Sorry, I don't remember testing this PR. I can add that to my todo list

@ryantrem
Copy link
Member Author

Found a private chat between us back in January and you said:

I tried to build&run your fix-render-order branch with PG 0.71 on Windows. It builds but crashes at startup. no log, no info on why it happens

No additional discussion after that, so I think that is as far as we got with testing on Windows and then the PR was put on hold.

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.

Main thread is blocked when rendering
4 participants