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

Improve frontend performance by avoiding useless renders #1321

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

LukasKalbertodt
Copy link
Member

The original goal was to speed up the search page (since it was rendered multiple times, uselessly), but these changes benefit all of Tobira. This generally cuts down on duplicate renders and useless work that is done. You can see this effect by adding console.log statements to ActiveRoute, a route of your choice like RealmPage and some other places. Do make sure to remove <StrictMode> for testing, as that doubles all renders again, making it harder to see. It's also helpful to add tokio::time::sleep(std::time::Duration::from_millis(1500)).await; to handle_api to easily see loading states.

This is a bit infuriating as it's still not perfect. But I cannot find a way to fix the last bit and searching for in-depth information on this stuff is super hard. I only find surface-level resources on this. Even blog posts which title contains "in-depth" or "deep dive" are laughably shallow. The react docs are good in principle but leave out some behind the scene information that would aid with debugging. The React dev tools are also surprisingly unhelpful.

Here is some information on what is rendered when:

Initial load

----- send GQL
Render <Router>
Render <ActiveRoute />  @1
RealmRoute::render /lectures/d-chab/2015/autumn/529-0010-00L
Render <RootLoader>
----- network arrived
Render <ActiveRoute />  @1
RealmRoute::render /lectures/d-chab/2015/autumn/529-0010-00L
Render <RootLoader>
Render <RealmPage /> @9 Chemistry

That's actually perfect as far as I can tell. First the request is sent, then the router routes and tries to render the active route, which leads to <RootLoader> suspending (then showing <InitialLoading />). Once the response arrives, the child of <Suspense> (<ActiveRoute>) is rerendered, and this time RootLoader does not suspend, so the RealmPage is rendered (only once!).

Clicking on a link

----- send GQL
Render <Router> {isPending: true, ar: '@1', sar: '@2', nav: '@3', lis: '@4', …}
Render <Router> {isPending: false, ar: '@10', sar: '@2', nav: '@3', lis: '@4', …}
Render <ActiveRoute />  @10
RealmRoute::render /lectures/d-chab/2015/autumn
Render <RootLoader>
Render <Router> {isPending: false, ar: '@10', sar: '@2', nav: '@3', lis: '@4', …}
Render <ActiveRoute />  @10
RealmRoute::render /lectures/d-chab/2015/autumn
Render <RootLoader>

----- network arrived
Render <RootLoader>
Render <RootLoader> after relay 1 @6 @7 @8
Render <Router> {isPending: false, ar: '@10', sar: '@2', nav: '@3', lis: '@4', …}
Render <ActiveRoute />  @10
RealmRoute::render /lectures/d-chab/2015/autumn
Render <RootLoader>
Render <RootLoader> after relay 0 @11 @12 @13
Render <RealmPage /> @14 Autumn

Good: the GQL request is sent first thing. Then the router is rerendered because isPending switches to true, which causes the loading indicator to do its thing. But:

Before the response arrives, we already have one duplicate rendering. Why is the router rendered twice with exact same props and all? This goes down to the RootLoader twice, which suspends both times I suspect. I actually know how to make it go away: Removing <LoadingIndicator> completely gets rid of one duplication (the last four lines before network arrives). But all the ways I tried to add loading indicator back again resulted in a duplicate render again. But oh well, at least the actual <RealmPage> is not rendered, so it should all be fairly cheap.

Secondly, after the response arrives, the RootLoader is rendered for an unknown reason. If it weren't for the last commit, that would rerender the RealmPage with the previous data! No idea why. And then everything is freshly rendered starting with Router down to RealmPage.

So... as far as I can see, the actual page (RealmPage in this case) is not rendered uselessly. Which is already a success. But still, weird stuff is happening and as I said, it's infuriating.

The `isTransitioning` state was part of the router context meaning that
all components subscribed to the context were rerendered when the state
changed. That is undesirable as this caused many useless renders. One
could still keep one context and libraries like `use-context-selector`,
but that seemed overkill for this one usecase. I also considered passing
`LoadingIndicator` to `Router` which works, but `header/Search` also
uses `isTransitioning`. This will likely be removed when I removed
instant search, but to keep this a well contained PR and for future
flexibility, I opted for the "two contexts" solution.
The context object changed on every render of `<Router />` meaning that
all subscribers of `RouterContext` were always rerendered.
There are still some renders that seem useless to me but that I cannot
find a way to get rid of. It's infuriating really. This change at least
makes sure that the expensive part (the actual page content) is not
rendered uselessly. Again, it would be nicer to make sure, `ActiveRoute`
and `RootLoader` aren't rendered uselessly in the first place, but I
couldn't make that happen.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant