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: avoid new URL() in hot path #12654

Merged
merged 2 commits into from
Mar 29, 2023
Merged

perf: avoid new URL() in hot path #12654

merged 2 commits into from
Mar 29, 2023

Conversation

patak-dev
Copy link
Member

Description

@ArnaudBarre mentioned that new URL is a bottleneck in his tests. We don't really need to use new URL here because to resolve the Id we are already using cleanUrl which is a simple regex.

This path was first using parseUrl, then changed by @sapphi-red to use new URL here

@sapphi-red maybe this should also have an effect on your perf benchmark, although it should have less impact once we merge


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Mar 29, 2023

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

@patak-dev
Copy link
Member Author

@ArnaudBarre maybe you can test your own project to see if this helps you? I checked and we don't have any other new URL call in a hot path after this one.

@patak-dev patak-dev added the performance Performance related enhancement label Mar 29, 2023
bluwy
bluwy previously approved these changes Mar 29, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Nice refactor. I noticed URL in the cpuprofiles too.

@ArnaudBarre
Copy link
Member

On a 3.2s timeline in speedscope (cold start):
Screenshot 2023-03-29 at 19 07 01
Screenshot 2023-03-29 at 19 13 44

So this is a big 1% speedup, worth it!

@patak-dev patak-dev merged commit f4e2fdf into main Mar 29, 2023
@patak-dev patak-dev deleted the perf/avoir-new-url branch March 29, 2023 19:19
@sapphi-red
Copy link
Member

Nice 👍

This reduced about 150ms.

Before

Vite  startup time: 4926ms (including server start up time: 497ms)
Vite  Root HMR time: 35ms
Vite  Leaf HMR time: 14ms
Vite (swc)  startup time: 3572ms (including server start up time: 241ms)
Vite (swc)  Root HMR time: 33ms
Vite (swc)  Leaf HMR time: 12ms

After

Vite  startup time: 4753ms (including server start up time: 485ms)
Vite  Root HMR time: 34ms
Vite  Leaf HMR time: 15ms
Vite (swc)  startup time: 3402ms (including server start up time: 237ms)
Vite (swc)  Root HMR time: 33ms
Vite (swc)  Leaf HMR time: 13ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants