-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: module graph url shortcuts #12635
Conversation
Run & review this pull request in StackBlitz Codeflow. |
I tried to cache the |
Thanks! The result of running https://github.com/sapphi-red/performance-compare/tree/65d51a32b0282593f1b75def25075a02b28f6397 on my machine. Without this PR
With this PR
|
That is a massive difference. I think the example isn't reflecting a real app well, but this would indeed help. @sapphi-red maybe you could test with https://github.com/sun0day/vite-2.7-slow on your computer? Marking the PR as ready for review. Given that there is such a big performance boost, I think we should consider merging this. |
This PR improves performance of the non-pre-bundled files. Because that repo only includes a few project files, this PR won't have any impact.
I think it's still a bottleneck, but not as bad as before. |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
I forgot to pass This is the correct one. This time, I took with SWC one too. |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
@sapphi-red would it be possible to run the tests again on your machine to see if the speed-up was due to the current changes in the PR or by the other optimization that I removed (and if that helps I can send in a separate PR, but it was generating an error in vite-plugin-vue for some reason)? |
Here's the result 🙂
|
Thanks @sapphi-red! So it seems that the biggest perf boost is from the rawUrl cache, but the resolving short-circuit also helps (I'll send another PR and we can check there why plugin-vue was failing). @bluwy moved the cache to a new |
/ecosystem-ci run previewjs |
Thanks for fixing tsconf.check.json @ArnaudBarre! @sapphi-red could you check this looks good to you too? |
I think this is good to merge after making the cache depend on the |
After vitejs/vite@
|
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
@ArnaudBarre @bluwy @sapphi-red looks like using private identifiers is making other CIs fail. From the SvelteKit one:
Should we try to push for everyone to update to ES2020? Maybe I should revert to using WeakMaps for now and we can push this change in Vite 5. Still is strange because we already have ES2020 in our |
Private identifier are supported in node12, so I really don't think we will break anyone, this is just misconfiguration like we had ourself. I think we should push for a fix on Svelte CI and continue using the proper JS feature in this PR |
I agree, although I think we don't win much by starting to use it now compared to in a few months. And we can work with others to ensure that they are all ready. |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Yeah, it looks good to me 👍 |
Tests seems to be more flaky after this, but I think we may be uncovering issues with the tests or other race conditions. Several issues in the ecosystem tests thought. I think there is a lot of value in this extra cache, worth exploring what is going on here |
It looks like we're also doing a double I think we don't notice these as much because the |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Same projects are failing in main https://github.com/vitejs/vite-ecosystem-ci/actions/runs/4567006041, probably due to changes to vite-plugin-vue today. I think we can merge this one to test it during the beta |
Description
Implement shortcuts based on this idea by @sapphi-red
It looks like there are opportunities here. But I don't see an improvement in the tests we were running. Sending the PR as an exploration, maybe a similar one should be merged if we can prove it helps.
What is the purpose of this pull request?