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): write templates in single sync step + improve logs #22384

Merged
merged 5 commits into from Jul 30, 2023

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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

This PR bundles three performance-focused improvements to template compilation and writing:

  1. it writes all template files in synchronous step to avoid additional runtime overhead of cascading HMRs from vite/webpack
  2. It logs errors granularly rather than hard erroring out, doesn't block the start of Nuxt, and makes it clearer what the issue was
    CleanShot 2023-07-29 at 05 14 47@2xCleanShot 2023-07-29 at 05 15 07@2x
  3. It logs the time each template takes to compile in debug mode (or if any template takes more than 500ms - we could potentially lower this)

πŸ“ Checklist

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

@stackblitz
Copy link

stackblitz bot commented Jul 29, 2023

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

}
}))

for (const write of writes) { write() }
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, using sync methods, blocks (our) main js render loop but it won't speed up v8 i/o threads. Have you tried with this new batching technique ++ async Promise.all to see timing differences?

Copy link
Member Author

@danielroe danielroe Jul 29, 2023

Choose a reason for hiding this comment

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

I've done some tests but the performance benefit I'm looking for here is reducing cascading hmr rather than reducing the time required to write the files.

Copy link
Member

@pi0 pi0 Jul 29, 2023

Choose a reason for hiding this comment

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

Yes, I understand that main point πŸ‘πŸΌ But i suppose (native) HMR watchers are not in the same event loop of main javascript process we are writing files from (unless our receiver is sync again including vite). If either method is faster, it reduces final chance to have duplicate HMR events but if you already tried both, feel free to ignore this comment :)

Copy link
Member Author

Choose a reason for hiding this comment

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

it may be an unnecessary optimisation. I tried both without appreciable time difference, and flagged up the fact that we are actually not watching the buildDir with vite. I will have a look to see if we actually should be watching the directory. Not doing so might be causing some occasionally issues with vite-node cache, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you have a good point here and we should definitely iterate on this. I'd be very open to rethinking this if you notice any issue..

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

That makes sense to me, I think it's worth trying. Shall we better leave a comment inside to explain it's on purpose?

@danielroe danielroe changed the title perf(nuxt): write all template files in a synchronous step perf(nuxt): write templates in single sync step + improve logs Jul 30, 2023
@danielroe danielroe merged commit a2b5d31 into main Jul 30, 2023
29 checks passed
@danielroe danielroe deleted the perf/templates branch July 30, 2023 16:14
@github-actions github-actions bot mentioned this pull request Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants