-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix Table
blocking main thread
#2462
base: main
Are you sure you want to change the base?
Conversation
@r100-stack When you get the chance, can you take a look and share any preliminary feedback? I will make this ready for proper review later. |
8628d44
to
9cf9937
Compare
9cf9937
to
46126b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r100-stack When you get the chance, can you take a look and share any preliminary feedback? I will make this ready for proper review later.
I took a look at the PR. Maybe it's something to do with my machine, but the time it takes to load the second table actually increases after this PR? For me, the main branch loads the second table in around 4.5 seconds whereas this branch takes around 6 seconds.
I didn't enable throttling. I tested using the App.tsx in this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main purpose of this PR is to unblock the main thread. So, the UI should no longer feel "laggy".
We're rendering 1000 rows in the playground, so it's expected to be slow. But those 6 seconds should feel a lot smoother now. During that time, the user interact with other parts of the page e.g. switching back to the original tab to cancel loading. (Or at least that's the idea.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, I see.
During that time, the user interact with other parts of the page e.g. switching back to the original tab to cancel loading. (Or at least that's the idea.)
I'm a little confused about this part. Because while it is loading, if I click on the change theme button on the top right, the theme changes only after the second table is loaded. This is also true when using the user's sandbox from #2444.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a helpful observation. I was able to additionally confirm using an uncontrolled input that the main thread is still blocked while the expensive Table is rendering. Will look into this next week.
Changes
WIP
Fixes #2444
After some investigation, I found that the issue was the use of
flushSync
inShadowRoot
(which gets rendered by everyDefaultCell
). Since a Table can have many cells, this ends up blocking the main thread for a long period. Removing theflushSync
call allows React to yield to the browser in between rendering different components.flushSync
was added as a default to ensure that the content gets rendered as soon as the shadow-root is attached. This PR makes this behavior configurable so that it can be turned off for special cases like theDefaultCell
where the performance cost outweighs the benefits of synchronous layout.Testing
Recreated the sandbox from #2444 in vite playground and manually tested using Chrome Dev Tools.
Preview: https://itwin.github.io/iTwinUI/2462/vite/
Docs
Pending.