Improve ssr hydration performance #5623
Conversation
- Fixes sveltejs#4308 by avoiding de- and reattaching nodes during hydration - Turns existing append, insert and detach methods into "upserts" - Logs hydration errors to the console The new "hydration mode" was added in order to maintain the detach by default behavior during hydration. By tracking which nodes are claimed during hydration unclaimed nodes can then removed from the DOM at the end of hydration without touching the remaining nodes.
Avoids hydration errors caused by whitespace diffs
|
Hmm. Upon further testing I keep running in to new issues. Might need to port all of the white space fixes from #4309 in order to address this. |
|
Tweaked the error logging slightly. The errors I encountered before were due to an error when linking the package locally, causing rollup to bundle multiple versions of Svelte. After re-linking the package those errors went away. |
|
Hey there! I think my main question here can be boiled down to 'What's a hydration error?' Hydration should be able to deal with whatever initial DOM you throw at it, and it being different from what the component expects doesn't necessarily indicate any sort of error or anything that users should be warned about. In what situations would your code print these errors to the console? And, when they are printed, does hydration still do its job? If there aren't the same guarantees, then this would be a breaking change, and we can't just call whatever this does the same thing as the current hydration. If there are the same guarantees, what purpose do the warnings serve? |
|
If the error message is not supposed to ever be printed, but is just there so that we are alerted if the code ever enters an unexpected state then maybe you can also hook into |
|
Thank you for reviewing this! @Conduitry Thats a good question. I guess a more correct term in a Svelte context would be hydration missmatch rather than an error. I'd define it as when the markup generated on the server unexpectedly differs from what's rendered on the client. This PR shouldn't change the current behaviour, other than by adding those errors/warnings. Differences between the initial markup and whats rendered client side are treated the same way as before and all tests concerning this pass. Re the errors/warnings: It does makes sense to warn about this as miss-matched markup/DOM trees will cause performance and SEO issues due to extra cpu work and layout thrashing. Without these messages its more or less impossible to detect unintended differences and address them. We could limit them to dev-only, but it's really in production that they will hurt you... Finally, there is also an argument to be made for an even stricter hydration algorithm that is optimised for the common case of server and client markup being 100% or 99% the same instead of assuming that they will always differ. I started drafting an RFC outlining what that would look like the last week. I'll post a link here once that's ready. |
Isn't SSR + hydration the most common use case? |
It is, but we can't change the behavior of existing hydration without it being a breaking change. And besides, there will be legitimate cases where the SSR HTML will be necessarily different from the browser HTML. For example, any text that will depend on what the user's timezone is set to on their computer, which is impossible for the server to know. |
What about keeping the current behavior as-is and passing an option to the compiler so that hydration will expect the same DOM structure (excluding minor changes like text content)? Honestly, not sure if this is feasible or makes sense though. |
| } else { | ||
| // Ignore hydration errors caused by empty text nodes | ||
| if (node.nodeType !== 3 || (node.data !== ' ' && node.data !== '')) { | ||
| console.error(`Hydration error: Expected node "${name}" but found`, node); |
There was a problem hiding this comment.
we shouldn't call this an "error". even calling it a "warning" seems a bit strong since it's intended to work this way at least sometimes. I'd probably make it an info
|
@jonatansberg are the warning feature and performance improvements tied to each other? What do you think about splitting the two into different PRs to help get the performance improvements in faster? Also, it'd be helpful if you could describe the use case for the warnings so that we can understand whether they might best be a dev mode feature, etc. |
I think there was some suggestion of that in #4308. It's probably better to keep that discussion there or elsewhere since it's not really related to what this PR is trying to do. But it's something we've considered, but don't have an answer for yet because there have been some proposals for changing other mechanics and we want to consider the various proposals holistically. I'm afraid there's not much more I can share at this point. We're probably not likely to make much progress on this are in the short-term even in terms of answering questions and considering proposals as the focus is on getting SvelteKit to a good place. I think this PR is the one hydration improvement that seems like it has a potential path forward to me at this point because it's backwards compatible and doesn't require us to commit to new flags or options without having had time to fully consider how it might interact with other ideas for Svelte core. |
|
From an SEO perspective I believe that this model will fix many of the LCP issues and delayed indexing issues due to hydration thrashing. |
|
Regarding the console warnings/errors - I'd probably opt for not including them at all right now. They're not really errors, they're a situation that is already handled and would apparently continue to be handled. And there are currently entirely valid situations where the hydrated HTML wouldn't match the SSR'd HTML, and if we started displaying these warning in production, people would absolutely ask for a way to disable them, which would be tricky to do. I'm not entirely averse to having these be dev-only messages (which could be done by making separate |
| } | ||
| } | ||
|
|
||
| return svg ? svg_element(name) : element(name); | ||
| } | ||
|
|
||
| export function claim_text(nodes, data) { | ||
| for (let i = 0; i < nodes.length; i += 1) { |
There was a problem hiding this comment.
why was this for loop changed to processing only the first element?
Background
This PR improves SSR hydration performance through minimizing DOM mutations during hydration by avoiding de- and reattaching nodes during hydration. This should fix #1067, #4308 and parts of #5108.
In order for this to work I added a new new "hydration mode". This way it's possible to maintain the detach by default behavior during hydration without modifying the generated component output. There might be better ways to do this if we reconsider fragment creation (as suggested in #3898 and #4219) but that would be a significant undertaking.
The existing append, insert and detached methods have been modified to track which nodes are claimed during hydration. That way unclaimed nodes can then removed from the DOM at the end of hydration without touching the remaining nodes.
In order for this to work the append, insert and detach methods have also been made idempotent (i.e. "upserts") in order to allow attaching an already attached DOM node.
Finally I also added some basic logging of hydration errors to the console.
Results
Initial local benchmarks against johnells.se (which runs on Sapper) shows a 50% reduction of LCP when running Lighthouse, which results in decent PageSpeed score improvement.
Next steps