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

SSR'd diffing taking ~2x the time of non-SSR'd boot. #445

Closed
paullewis opened this Issue Dec 12, 2016 · 19 comments

Comments

Projects
7 participants
@paullewis

paullewis commented Dec 12, 2016

I've just pushed a blog post, and I've noticed that in 7.1.0 I'm seeing SSR'd content boot take upto 2x as long as Client-Side: https://aerotwist.com/blog/when-everything-is-important-nothing-is/

Looking at a timeline it seems it's mostly in the diffing.

screen shot 2016-12-12 at 3 39 57 pm

@developit

This comment has been minimized.

Owner

developit commented Dec 12, 2016

Very strange that it's stripping nodes for sure. I can see how that would take 2x longer. There's some combination of components in the hierarchy that is being considered a mismatch when doing the initial render. I'm diving into your repo to take a look!

@developit developit self-assigned this Dec 12, 2016

@developit

This comment has been minimized.

Owner

developit commented Dec 12, 2016

Discovered the source! Since the HTML you're treating as the server render contains whitespace, Preact is diffing against that instead of the firstChild of <section class="post">. It's discarding the entire tree.

@matthewp

This comment has been minimized.

Contributor

matthewp commented Dec 12, 2016

Interesting findings. Since JSX treats whitespace differently than HTML is there anything you can do about this? Maybe ignore whitespace if it seems to be unimportant?

@developit

This comment has been minimized.

Owner

developit commented Dec 12, 2016

Trying to look into the edge cases. It'd be something like "ignore whitespace nodes adjacent to element boundaries" (so leading/trailing whitespace, and whitespace-only Text node siblings of Elements).

@scurker

This comment has been minimized.

Contributor

scurker commented Dec 12, 2016

What would be the most efficient way of helping to eliminate this in the short term? Would stripping white space on the SSR'd content do it? (i.e. with something like htmlmin)

@developit

This comment has been minimized.

Owner

developit commented Dec 12, 2016

Yes. Using minified HTML entirely solves the issue.

I think this was dormant for a while because almost everyone server-renders using preact-render-to-string, which does not include whitespace by default. I believe Paul's demo is using HTML rendered from some other mechanism, and it's pretty-printed in a way that makes diffing against it quite difficult (meaningful whitespace VS non).

So, definitely something I'm looking into (kinda killed my day so far!) but the TL,DR is that server rendering via preact-render-to-string does not produce this issue.

@scurker

This comment has been minimized.

Contributor

scurker commented Dec 12, 2016

Thanks @developit I didn't notice that preact-render-to-string already did that.

@developit

This comment has been minimized.

Owner

developit commented Dec 12, 2016

Indeed! There's an option to turn on pretty whitespace (really just used because it can output JSX, which gets used in preact-jsx-chai), but it's off by default for this reason.

@developit developit added the important label Dec 12, 2016

@aweary

This comment has been minimized.

aweary commented Dec 13, 2016

Does it make sense to supporting diffing against HTML rendered by something other than Preact? It seems like it would be the responsibility of the user to make sure the HTML is identical to what preact-render-to-string would output, rather than introducing overhead internally to handle normalization.

@developit

This comment has been minimized.

Owner

developit commented Dec 13, 2016

To me it sortof comes down to whether it can reasonably be supported without adding much overhead. I've been wrestling with that issue and I do have a fairly elegant solution that meets the need and only seems to add 30-40 bytes of overhead (not 0, but not a ton).

Another option we have at our disposal is modularity. For example, to fix the whitespace issue Paul's demo showed, you could run this standalone function against the DOM prior to invoking Preact:

// process(document.body);
function process(node) {
  if (node.nodeType==3 && !node.nodeValue.trim()) node.remove();
  for (var i=(node.childNodes || '').length; i--; ) process(node.childNodes[i]);
}

This is essentially the whitespace fix I've applied, but as a standalone module.

Update: that snippet is now on npm: developit/strip-dom-whitespace.

So my question to everyone who has an interest here would be: what is this feature worth? Personally I think I can see the value, but I'm just a single stakeholder.

@matthewp

This comment has been minimized.

Contributor

matthewp commented Dec 13, 2016

I'm not a Preact user, so take this as a low value comment, but I think it's the correct thing to do. I don't think using Preact should require that you use Node.js and use Preact for server templating. Requiring that HTML be identical, including TextNodes, imo restricts Preact's usefulness.

@aweary

This comment has been minimized.

aweary commented Dec 13, 2016

I'm not saying that it should require the HTML to be generated by Preact, but I do think it's reasonable that the responsibility should be on the user to make sure they're maintaining consistency with what Preact expects.

Deferring to an optional package that the user should include does exactly that, so I'm 👍 with that!

@developit

This comment has been minimized.

Owner

developit commented Dec 13, 2016

Bah I'm so undecided on this. It's such a small amount of code to support the use-case. I'm going to keep running benchmarks and if I find something horrible about it at least that's a line in the sand. Right now I can't decide.

@developit developit added this to the 7.2.0 milestone Dec 14, 2016

@chrisdavies

This comment has been minimized.

chrisdavies commented Dec 18, 2016

For what it's worth, I'm of the same opinion as @aweary. I think it's fine if preact just clearly documents what is necessary to get isomorphic behavior working properly, but then leaves it up to the developer to make sure they do this correctly.

@developit

This comment has been minimized.

Owner

developit commented Dec 18, 2016

Plus there's a module now that handles it for you, just have to invoke it on document.body.

@lukeed

This comment has been minimized.

Collaborator

lukeed commented Jan 9, 2017

FWIW, Inferno gets around this by removing all whitespace during the compilation step via its babel-plugin-inferno.

@developit

This comment has been minimized.

Owner

developit commented Jan 9, 2017

That happens in the default JSX plugin too. Issue we ran I to here was whitespace in the server rendered HTML DOM. JSX rendered by preact-render-to-string never has extra whitespace, but this HTML wasn't re served by preact-render-to-string. Funky case but the fix was simple enough.

@developit developit modified the milestones: 7.2, 8.0 Mar 9, 2017

@developit

This comment has been minimized.

Owner

developit commented Mar 9, 2017

The last bit of this is going into 8.0, which involves having Preact's diff ignore whitespace-only nodes why doing SSR hydration.

@developit developit added the has fix label Mar 9, 2017

@developit developit added this to Code-Complete in 8.0 Mar 9, 2017

@developit developit moved this from Code-Complete to Merged in 8.0 Apr 6, 2017

@developit

This comment has been minimized.

Owner

developit commented Apr 6, 2017

Fixed in 8!

@developit developit closed this Apr 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment