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: reduce _prerenderRoutes memory footprint #1529

Closed
wants to merge 3 commits into from

Conversation

Hebilicious
Copy link
Contributor

@Hebilicious Hebilicious commented Aug 4, 2023

πŸ”— Linked issue

Related #1535
Related #1480
Related nuxt/nuxt#22449

❓ 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

Based on @simkuns patch. Internal _prerenderedRoutes now only contains route and fileName to reduce the memory footprint

πŸ“ Checklist

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

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #1529 (abd59cf) into main (451d314) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1529   +/-   ##
=======================================
  Coverage   76.25%   76.25%           
=======================================
  Files          73       73           
  Lines        7478     7481    +3     
  Branches      734      734           
=======================================
+ Hits         5702     5705    +3     
  Misses       1775     1775           
  Partials        1        1           
Files Changed Coverage Ξ”
src/prerender.ts 87.17% <100.00%> (+0.10%) ⬆️

@danielroe
Copy link
Member

Nice! ❀️

@Hebilicious Hebilicious changed the title perf: preprend routes and trim _prerenderRoutes perf: prepend routes and trim _prerenderRoutes Aug 4, 2023
@parzychu
Copy link

parzychu commented Aug 4, 2023

Great! <3

@Hebilicious Hebilicious requested a review from pi0 August 4, 2023 15:10
@Hebilicious Hebilicious marked this pull request as ready for review August 4, 2023 15:10
src/prerender.ts Outdated
@@ -213,7 +215,11 @@ export async function prerender(nitro: Nitro) {
);
for (const _link of extractedLinks) {
if (canPrerender(_link)) {
routes.add(_link);
if (_link.endsWith("_payload.json")) {
Copy link
Member

Choose a reason for hiding this comment

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

sorry but i cannot let this to be merged in nitro. please consider proposing framework agnostic solutions AND make sure there is a nitro minimal reproduction that makes sure it actually proves and solves an issue instead of hiding root causes.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Requirements:

  • Framework agnostic implementation
  • Nitro minimal reproduction to make sure there is an issue with Nitro itself, not downstream framework (Nuxt) or modules.

As a Nuxt team member and nitro maintainer, I tend to fix the root cause of issues not patching them around.

@pi0 pi0 marked this pull request as draft August 4, 2023 17:32
@Hebilicious
Copy link
Contributor Author

Hebilicious commented Aug 4, 2023

Thanks for the feedback @pi0

I'm open to suggestions for a framework agnostic design.
What if we accept a regex from the settings likenitro.options.prerender.prependExtractedLinkRegex?
This can then be used by Nuxt to achieve such a behaviour.

Edit @danielroe proposed a hook to do that, I'll refactor and open another PR

@pi0
Copy link
Member

pi0 commented Aug 4, 2023

(i am working on reproduction and alternative fix for content issue)

@Hebilicious Can you please explain more why we would need to reorder prerender routes and how it relates to a possible fix?

@Hebilicious
Copy link
Contributor Author

Hebilicious commented Aug 4, 2023

(i am working on reproduction and alternative fix for content issue)

@Hebilicious Can you please explain more why we would need to reorder prerender routes and how it relates to a possible fix?

This re-ordering was based on this comment that included a nice heap profile.
#1480 (comment)

However with daniel suggestion, I now think its better to expose a hook (before runParallel) so that frameworks (nuxt, modules ...) can inspect and re-order routes if they wish to do so.

For testing we could test the perf differences with nuxt/nuxt#22465 and #1529 and see how much re-ordering with payload routes first would impact things.

@Hebilicious Hebilicious changed the title perf: prepend routes and trim _prerenderRoutes perf: trim _prerenderRoutes Aug 4, 2023
@Hebilicious Hebilicious marked this pull request as ready for review August 4, 2023 18:09
@Hebilicious Hebilicious changed the title perf: trim _prerenderRoutes perf: reduce _prerenderRoutes memory footprint Aug 4, 2023
@Hebilicious Hebilicious requested a review from pi0 August 4, 2023 18:10
@pi0
Copy link
Member

pi0 commented Aug 4, 2023

Thanks for PR and explanations @Hebilicious and pin pointing the exact issue @danielroe i have made a better rework in #1536 which seems to fix (reproducable) issue.

@Hebilicious I would recommend to try nuxt edge + nitro edge with both fixes (LRU cache and #1536) and rerun any previous benchmarks again to make sure sorting with fixes makes a difference. If it does, we already have prerender:routes(routes) hook that get's called before parallel run and can be used to reorder. We might think of another hook too prerender:extractedlinks (or similar that is called inside generateRoute) if sorting of later discovered. I think generally would be a nice addition but again, please test.

@pi0 pi0 closed this Aug 4, 2023
@pi0 pi0 deleted the perf/preprend-payload-routes branch August 4, 2023 18:53
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.

4 participants