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(kit, schema, nuxt): rework startsWith to direct index #24744

Merged
merged 18 commits into from Dec 29, 2023

Conversation

GalacticHypernova
Copy link
Contributor

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

JS's startWith is known to have a performance downgrade compared to traditional character checking (you can see more here how vite also substituted those). And indeed, the performance increase is very much noticeable (~20% increase in performance in single character checks and ~15% increase in performance from starting 2 characters, increasing further as more characters are introduced).
Benchmark results:

const nuxt={
    _ignorePatterns:["abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!","abcdefghijklmnopqrstuvwxyz*!"]
}
console.time("currentSingle")
for (let index = 0; index < 100000; index++) {
    nuxt._ignorePatterns.map(p => p.startsWith('*'))
}
console.timeEnd("currentSingle")
console.time("PRSingle")
for (let index = 0; index < 100000; index++) {
    nuxt._ignorePatterns.map(p=>p[0]==="*")
}
console.timeEnd("PRSingle")
    console.time("currentM")
for (let index = 0; index < 100000; index++) {
    nuxt._ignorePatterns.map(p => p.startsWith('!*'))
}
console.timeEnd("currentM")
console.time("PRM")
for (let index = 0; index < 100000; index++) {
    nuxt._ignorePatterns.map(p => p[0] === '!' && p[1] === '*')
}
console.timeEnd("PRM")

image

Now, I know this hurts readability a bit, which is why to make a reasonable tradeoff (if you guys would like to), it is possible to only substitue for single character and double character check, and keep the rest with startsWith. I am curious to hear your opinion on how many characters you would like to substitue (do note that startsWith with 10 characters would be many times slower than === with 10 characters)

πŸ“ Checklist

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

Copy link

stackblitz bot commented Dec 14, 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 14, 2023
@GalacticHypernova GalacticHypernova changed the title perf(kit): rework startsWith to direct index perf(kit, schema): rework startsWith to direct index Dec 14, 2023
@GalacticHypernova GalacticHypernova marked this pull request as draft December 14, 2023 11:04
@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Dec 14, 2023

Making this a draft too to make as many changes as possible. How many characters would you say would be a fair tradeoff between readability and performance? I don't mind simply adding clear comments above each change so that even newcomers would understand 10 character checks, and allow for maximal speed and efficiency.

@GalacticHypernova GalacticHypernova changed the title perf(kit, schema): rework startsWith to direct index perf(kit, schema, nuxt): rework startsWith to direct index Dec 16, 2023
@GalacticHypernova
Copy link
Contributor Author

I do believe this is all of it, I had to test it very thoroughly, so far I have gotten reliable consistent results (in favor of this PR's method) with up to 2 characters. Of course, I will continue testing to see if this can be improved further.

@GalacticHypernova GalacticHypernova marked this pull request as ready for review December 28, 2023 20:03
@danielroe danielroe merged commit c2b94d4 into nuxt:main Dec 29, 2023
34 checks passed
@github-actions github-actions bot mentioned this pull request Dec 29, 2023
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