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): prefer iterating rather than Object.fromEntries #24953

Merged
merged 24 commits into from Jan 9, 2024

Conversation

GalacticHypernova
Copy link
Contributor

@GalacticHypernova GalacticHypernova commented Dec 29, 2023

πŸ”— Linked issue

❓ 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

Many parts of nuxt require some sort of object manipulation, and therefore use Object.fromEntries and some more filter methods. This, especially in larger inputs, can really slow it down. This PR solves these issues by eliminating unnecessary iterations.

πŸ“ Checklist

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

Copy link

stackblitz bot commented Dec 29, 2023

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

@GalacticHypernova GalacticHypernova marked this pull request as draft December 29, 2023 10:56
@github-actions github-actions bot added the 3.x label Dec 29, 2023
@GalacticHypernova GalacticHypernova changed the title perf(nuxt): refactor Object.fromEntries perf(nuxt, vite): refactor Object.fromEntries Jan 4, 2024
@GalacticHypernova GalacticHypernova marked this pull request as ready for review January 4, 2024 11:15
@GalacticHypernova GalacticHypernova marked this pull request as draft January 4, 2024 11:18
@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Jan 4, 2024

Sorry this took so long, personal life has been very demanding!
I would like to hear your opinion on whether this PR should be an "mono-perf PR" and kept around to group all improvements (kind of like a monorepo but in PR form) or separate them by topics (like this one currently is for Object.fromEntries)

@GalacticHypernova GalacticHypernova marked this pull request as ready for review January 4, 2024 18:58
@GalacticHypernova GalacticHypernova changed the title perf(nuxt, vite): refactor Object.fromEntries perf: refactor Object.fromEntries Jan 4, 2024
@GalacticHypernova
Copy link
Contributor Author

Thanks for the refactor!
I had planned to do it but got a bit caught up with something, sorry about that πŸ˜…
Although I do wonder why with reduce 2 checks were failing...

@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Jan 8, 2024

May I ask why the revert? I get that smaller arrays would provide less of a performance improvement, but it's an improvement nonetheless. If anything, we can further improve performance in the small entry in vite as the input is known, so if we just create the object manually we avoid all function calls like map, like so:

const toReplace = {
    "X": "Y"
//...
}
//...
replace({...toReplace})

And I'm not even sure, could the bundles be considered small? Is it not user-input dependant?
(Also I apologize for the bad code indent, doing this on mobile isn't the easiest)

@danielroe
Copy link
Member

In the case of the analyse plugin, we are calling Object.entries to get an iterator. It's a one-time call (and only when analysing). And it is more readable.

For the other one, it's also a one-time setup call called when Nuxt starts with a very small number of non-user controlled entries. Also negligible performance difference vs readability.

@danielroe danielroe changed the title perf: refactor Object.fromEntries perf(nuxt): prefer iterating over objects rather than using Object.fromEntries Jan 9, 2024
@danielroe danielroe changed the title perf(nuxt): prefer iterating over objects rather than using Object.fromEntries perf(nuxt): prefer iterating rather than Object.fromEntries Jan 9, 2024
@danielroe danielroe merged commit 3b94883 into nuxt:main Jan 9, 2024
34 of 36 checks passed
@github-actions github-actions bot mentioned this pull request Jan 9, 2024
@GalacticHypernova
Copy link
Contributor Author

Well, in that case I suppose it's reasonable. Thanks for the feedback!

@GalacticHypernova GalacticHypernova deleted the patch-13 branch January 9, 2024 17:08
@github-actions github-actions bot mentioned this pull request Jan 16, 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.

None yet

2 participants