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

Improved hydration performance by less reordering of nodes #6392

Closed
wants to merge 3 commits into from

Conversation

ehrencrona
Copy link
Contributor

As lots of issues (e.g. #1067, #6204) have discussed, there are more DOM changes than one might expect during hydration, which reduces performance.

This is an attempt at significantly reducing the number of DOM changes with a quite minor change to the code. In my tests, it speeds up hydration by around 25%.

As far as I can tell, the issue with the DOM manipulation during hydration is not so much that nodes are removed and recreated (because they generally are recycled), as that during the mount phase we will append them to their parent in order, which actually causes them to reorder:

If you have the child nodes A and B present in the SSR HTML and hydration starts by appending A, A will move to the end and the order becomes B followed by A. It will be reversed again when B is appended, thereby giving the desired order. These reorderings are quite slow.

This PR changes that by:

  • Storing a property on claimed nodes, signalling that they will be hydrated
  • Keeping track of an "insert pointer" on the parent node. In the example above, it would start at 0. As A is appended, it would notice that the node A is already at position 0 and therefore do nothing, except increase the pointer. Then B is appended, but B is already the node at position 1 so again there is nothing to be done. As it has reached the end of the node list, the pointer is removed from the node again.

In actually hydration, the picture is muddied by various less straightforward cases, but in 7 cases out of 8 in my tests, the algorithm above was able to find find the correct next node and therefore skip a DOM operation.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@benmccann
Copy link
Member

Thanks for this!

While #6204 based on the work by @jonatansberg had to be reverted due to a bug it seems like it would probably be even better performance than this if we were able to get it to work. It would also avoid issues like animations and videos restarting due to hydration which I believe would remain with this PR. I commented in #4308 (comment) that it looks like the way to do that might be to get @tanhauhau's #3766 merged first, which would offer a slight additional performance improvement and hopefully allow the reverted hydration PR to work as intended

CC @hbirler who submitted a similar PR in #6395. Maybe we should start a little group chat on Discord?

@hbirler
Copy link
Contributor

hbirler commented Jun 11, 2021

Sure! I can join the Discord.
Additionally, I believe that the optimal reordering strategy that I discuss in #6395 (which is inspired by this PR) actually covers the #6204 case. (In my small simple experiments with animation under SvelteKit with adapter-static, many animation restart issues were prevented.)

@benmccann
Copy link
Member

@ehrencrona should we close this PR based on your comment (#6395 (comment)) that the other PR is faster?

@ehrencrona
Copy link
Contributor Author

@ehrencrona should we close this PR based on your comment (#6395 (comment)) that the other PR is faster?

Yeah I think that's reasonable.

@ehrencrona ehrencrona closed this Jun 20, 2021
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