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

perf(nuxt): ensure renderHTMLDocument return more compact result #24888

Merged
merged 9 commits into from Jan 4, 2024

Conversation

Mini-ghost
Copy link
Contributor

πŸ”— Linked issue

Resolve #23085

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

I have attempted to minimize the size of the generated HTML by eliminating unnecessary line breaks where possible. Below is the HTML output from the current version after executing nuxi generate playground:

<!DOCTYPE html>
<html >
<head><meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="preload" as="fetch" crossorigin="anonymous" href="/_payload.json">
<link rel="modulepreload" as="script" crossorigin href="/_nuxt/entry.lirqHv3X.js">
<link rel="prefetch" as="script" crossorigin href="/_nuxt/error-404.tbGoJl1q.js">
<link rel="prefetch" as="script" crossorigin href="/_nuxt/vue.f36acd1f.eXCIzdvm.js">
<link rel="prefetch" as="script" crossorigin href="/_nuxt/error-500.9n_HQChl.js">
<script type="module" src="/_nuxt/entry.lirqHv3X.js" crossorigin></script></head>
<body ><div id="__nuxt"><div> Nuxt 3 Playground </div></div><script type="application/json" id="__NUXT_DATA__" data-ssr="true" data-src="/_payload.json">[{"state":1,"once":3,"_errors":5,"serverRendered":7,"path":8,"prerenderedAt":9},["Reactive",2],{},["Reactive",4],["Set"],["Reactive",6],{},true,"/",1703474852002]</script>
<script>window.__NUXT__={};window.__NUXT__.config={public:{},app:{baseURL:"/",buildAssetsDir:"/_nuxt/",cdnURL:""}}</script></body>
</html>

The adjusted versions are as follows:

<!DOCTYPE html><html><head><meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="preload" as="fetch" crossorigin="anonymous" href="/_payload.json">
<link rel="modulepreload" as="script" crossorigin href="/_nuxt/entry.HCIDZTxN.js">
<link rel="prefetch" as="script" crossorigin href="/_nuxt/error-404.fb9U4sbQ.js">
<link rel="prefetch" as="script" crossorigin href="/_nuxt/vue.f36acd1f.O0-pTQn_.js">
<link rel="prefetch" as="script" crossorigin href="/_nuxt/error-500.6TmM4WXY.js">
<script type="module" src="/_nuxt/entry.HCIDZTxN.js" crossorigin></script></head><body><div id="__nuxt"><div> Nuxt 3 Playground </div></div><script type="application/json" id="__NUXT_DATA__" data-ssr="true" data-src="/_payload.json">[{"state":1,"once":3,"_errors":5,"serverRendered":7,"path":8,"prerenderedAt":9},["Reactive",2],{},["Reactive",4],["Set"],["Reactive",6],{},true,"/",1703474792485]</script>
<script>window.__NUXT__={};window.__NUXT__.config={public:{},app:{baseURL:"/",buildAssetsDir:"/_nuxt/",cdnURL:""}}</script></body></html>

The extent of reduction is quite limited, primarily due to the fact that most of the <head> content is generated by @unhead/ssr, which might be beyond the scope of Nuxt.

I am open to feedback regarding whether this adjustment will yield significant benefits. Should the evaluation indicate that the drawbacks outweigh the benefits, please feel free to close this PR.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Dec 25, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the 3.x label Dec 25, 2023
@Mini-ghost Mini-ghost changed the title perf: ensure renderHTMLDocument return more compact result perf(nuxt): ensure renderHTMLDocument return more compact result Dec 25, 2023
Copy link

codspeed-hq bot commented Dec 25, 2023

CodSpeed Performance Report

Merging #24888 will degrade performances by 39.95%

⚠️ No base runs were found

Falling back to comparing Mini-ghost:chore/reduce-document-size (6e879da) with main (1a9fb57)

Summary

⚑ 1 improvements
❌ 3 regressions
βœ… 4 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main Mini-ghost:chore/reduce-document-size Change
❌ basic test fixture 451.9 ms 752.7 ms -39.95%
❌ basic test fixture (types) 355 ms 533.5 ms -33.44%
⚑ minimal test fixture (types) 136.3 ms 42.1 ms Γ—3.2
❌ minimal test fixture 39.2 ms 61.9 ms -36.67%

Copy link
Member

@manniL manniL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these changes will have a significant impact after text compression. GZIP/brotli should cover a lot of that already.

Would be interesting to see benchmarks on the standard application as well as "more real-life" project (e.g. something like https://github.com/nuxt/movies)

(As the refactoring here doesn't hurt readability significantly either I'm not opposed to merging btw)

@harlan-zw
Copy link
Contributor

harlan-zw commented Dec 26, 2023

re: extra spaces in <head > and <body >

Yes, please! Every time I see this I want to send a PR but have been avoiding the distraction

re: removing all line-breaks

In my experience, the saving % with compression isn't worth the loss of being able to easily debug page source. If you have benchmarks on the contrary then please share.

If you want smaller HTML then leaning into it and fully minifying the HTML will give you better results, which should already be easy to do via a nitro plugin.

Have no issues with this being an opt-in core feature though.

This is the same thinking I have with unjs/unhead#297, which I'm happy to accept as an opt-in if we want to go in that direction.

@Mini-ghost
Copy link
Contributor Author

Mini-ghost commented Dec 26, 2023

re: extra spaces in <head > and <body >

Yes, please! Every time I see this I want to send a PR but have been avoiding the distraction

re: removing all line-breaks

In my experience, the saving % with compression isn't worth the loss of being able to easily debug page source. If you have benchmarks on the contrary then please share.

If you want smaller HTML then leaning into it and fully minifying the HTML will give you better results, which should already be easy to do via a nitro plugin.

Have no issues with this being an opt-in core feature though.

This is the same thinking I have with unjs/unhead#297, which I'm happy to accept as an opt-in if we want to go in that direction.

Thank you for your valuable feedback.

However, I would like to seek some clarification: Are you suggesting the addition of an option in renderSSRHead or ssrRenderTags to remove newline characters?

Currently, I am integrating the Nitro plugin with html-minifier-terser to eliminate line-breaks and even to compact inline styles in my own projects. I believe it would be greatly beneficial if the core could support smaller HTML generation.

Nevertheless, I am curious about how the removal of newline characters might impact debugging, as I generally rely on Chrome DevTools for this purpose.

@Mini-ghost
Copy link
Contributor Author

Mini-ghost commented Dec 26, 2023

I wonder if these changes will have a significant impact after text compression. GZIP/brotli should cover a lot of that already.

Would be interesting to see benchmarks on the standard application as well as "more real-life" project (e.g. something like https://github.com/nuxt/movies)

(As the refactoring here doesn't hurt readability significantly either I'm not opposed to merging btw)

Thank you for your feedback. I may need to explore how to test this change within the nuxt/movies context. I anticipate that the difference might not be very obvious. However, when combined with the enhancements in @unhead/ssr, the impact could potentially become more discernible.

@harlan-zw
Copy link
Contributor

harlan-zw commented Dec 30, 2023

However, I would like to seek some clarification: Are you suggesting the addition of an option in renderSSRHead or ssrRenderTags to remove newline characters?

Yes, for both. Ideally, the head module would allow providing options to be used with the renderSSRHead function.

I think if you want to tackle this, you should do it in a separate PR and just keep this one simple, focusing on the extra unneeded spaces.

Nevertheless, I am curious about how the removal of newline characters might impact debugging, as I generally rely on Chrome DevTools for this purpose.

There are instances where you need to see the actual SSR DOM response instead of the client-hydrated DOM, especially when dealing with hydration issues.

@Mini-ghost
Copy link
Contributor Author

Yes, for both. Ideally, the head module would allow providing options to be used with the renderSSRHead function.

I think if you want to tackle this, you should do it in a separate PR and just keep this one simple, focusing on the extra unneeded spaces.

Sure! I will attempt to add an option to unjs/unhead#297 that allows developers to decide whether to remove newline characters based on their needs.

@Mini-ghost
Copy link
Contributor Author

@manniL

I have tried to compare the differences before and after this PR using the Nuxt Movies project. From the Response Headers provided by Chrome DevTools, I found

Before
Content-Length: 58148

After
Content-Length: 58143

Perhaps this cannot be classified as an improvement. πŸ˜…

@manniL
Copy link
Member

manniL commented Jan 3, 2024

@Mini-ghost Thanks for the effort! Yeah, not a huge thing but if it won't decrease perf or increase code complexity it is fine for me πŸ‘πŸ»

@danielroe danielroe merged commit 13c42d3 into nuxt:main Jan 4, 2024
34 checks passed
@github-actions github-actions bot mentioned this pull request Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nuxt doesn't minify html properly (does not remove line-breaks)
4 participants