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): don't dedupe fewer than two middleware/plugins #24718

Merged
merged 3 commits into from Dec 14, 2023

Conversation

GalacticHypernova
Copy link
Contributor

@GalacticHypernova GalacticHypernova commented Dec 12, 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

This PR returns early in the uniqueBy function that is used to gather all the unique plugins/middleware/etc whenever the input array is empty (which isn't unrealistic, if someone doesn't use any plugins for example) or contains a single file (guaranteed to be unique), avoiding all the computation afterwards and increasing performance in both cases by ~66.67%.
With the following benchmark I have gotten the following results:

 function uniqueByNonReg<T, K extends keyof T>(arr: T[], key: K) {
  if (arr.length < 2) {
    return arr;
  }

  const res: T[] = [];
  const seen = new Set<T[K]>();

  for (const item of arr) {
    if (seen.has(item[key])) {
      continue;
    }

    seen.add(item[key]);
    res.push(item);
  }

  return res;
}

 function uniqueBy<T, K extends keyof T> (arr: T[], key: K) {
  const res: T[] = []
  const seen = new Set<T[K]>()
  for (const item of arr) {
    if (seen.has(item[key])) { continue }
    seen.add(item[key])
    res.push(item)
  }
  return res
}

console.time('regularEmpty')
for(let i=0;i<100000;i++){
  uniqueBy([],'name')
}
console.timeEnd('regularEmpty')
console.time('nonRegularEmpty')
for(let i=0;i<100000;i++){
  uniqueByNonReg([],'name')
}
console.timeEnd('nonRegularEmpty')

console.time('regularSingle')
for(let i=0;i<100000;i++){
  uniqueBy([{name:'f'}],'name')
}
console.timeEnd('regularSingle')
console.time('nonRegularSingle')
for(let i=0;i<100000;i++){
  uniqueByNonReg([{name:'f'}],'name')
}
console.timeEnd('nonRegularSingle')

image

πŸ“ Checklist

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

Copy link

stackblitz bot commented Dec 12, 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 12, 2023
@GalacticHypernova GalacticHypernova changed the title perf(nuxt): return early for empty directories perf(nuxt): return early in empty directories Dec 12, 2023
@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Dec 12, 2023

I will look into optimizing the entire Set handling as well (perhaps rework it to a Map?), but from my current benchmarks the results are inconclusive and quite mixed (potentially because of it running online, which I will do some offline testing soon) so I have not added that part yet.

@GalacticHypernova
Copy link
Contributor Author

Update: the new update (the single file early return) yields even faster results, I will soon post new benchmark results

@GalacticHypernova GalacticHypernova changed the title perf(nuxt): return early in empty directories perf(nuxt): return early in empty or single filed directories Dec 13, 2023
@danielroe
Copy link
Member

Nuxt itself will always add more than 2 plugins, which means the additional check is likely to increase the time required when resolving plugins.

(Though I don't see the harm in adding this check as it might be relevant for middleware - Nuxt only adds one by default.)

@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Dec 13, 2023

which means the additional check is likely to increase the time required when resolving plugins.

Well, the check is O(1), so it doesn't add time at all compared to the time it saves (as you mentioned with the middleware). I'm not sure about the usability of that function in modules and the likes, but could people use it in their own modules or to register their own middleware/plugins?

@danielroe
Copy link
Member

danielroe commented Dec 13, 2023

It adds time (technically) because it will always be false when resolving plugins.

This function is only used to deduplicate plugins and middleware in this file.

@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Dec 13, 2023

I see. Allow me to benchmark this really quickly.

EDIT: The results are as follows:
image

Of course there is deviation a bit because it's online, but in the offline results I got the following:
image

So yes, while it does go up by 0.4 ms it saves in both empty and single filed directories ~66.67% and up to ~80%, which in comparison is a lot faster. The most is has ever gotten to was around ~+2ms to execution time in the multiple section but gets rid of ~4ms in the empty and ~7ms in the single.
Here is an example of such statistics:
image

Now, it's possible to make an overload of this function, but do you think it's worth it?

@danielroe danielroe changed the title perf(nuxt): return early in empty or single filed directories perf(nuxt): don't dedupe fewer than two middleware/plugins Dec 14, 2023
@danielroe danielroe merged commit 02306fd into nuxt:main Dec 14, 2023
35 checks passed
@github-actions github-actions bot mentioned this pull request Dec 14, 2023
@GalacticHypernova GalacticHypernova deleted the patch-12 branch December 15, 2023 14:35
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