-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: reduce bundle size for chunk imports #9491
Conversation
LGTM; this makes a lot of sense. And I can confirm this drastically reduces KBs and is not vite-plugin-ssr specific. |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this optimization! This looks good, but I'll wait for another team member to have their eye on this in case of something I've missed. I'll put this on the team board too so we don't forget.
Co-authored-by: Anthony Fu <[email protected]>
const __viteFileDeps = ${JSON.stringify( | ||
fileDeps.map((fileDep) => | ||
relativePreloadUrls ? path.relative(path.dirname(file), fileDep) : fileDep | ||
) | ||
)} | ||
function __viteMapDep(indexes) { | ||
return indexes.map((i) => __viteFileDeps[i]) | ||
}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some of our tests invokes __viteMapDep
on initialize, causing ReferenceError: Cannot access uninitialized variable.
in some cases. We might have to move __viteFileDeps
back in the function like before, or prepend this code instead.
Also this file needs to run pnpm format
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it back into __viteMapDep
and applied npnm format
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we prepend it instead? I am still not so sure about having it nested. Even JS engine might optimize it, it still needs to create new references and free the memory on each run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah I guess we could try prepend first. Thought it might have perf implications since we're prepending in a sourcemap, but I don't think it would matter actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another trick I can think about is:
function __viteMapDep(indexes) {
if(!this.files)
this.files = JSON.stringify({})
return indexes.map((i) => this.files[i])
}
Awesome! Some random thoughts. (Doesn't need to be included in this PR)
|
Confirming that these are great suggestions, but not planned as part of this PR. |
What are we waiting for to get this merged? |
I don't understand how you consider it "subjective". If this PR trade-off runtime performance for bundling size, then I wouldn't consider this to be a perf improvement. I believe it still belong to the scope of this PR. |
That test you showed is irrelevant to any real-world user scenario. It assumes millions of operations per second, where in real-world this function is called a few hundred times throughout the entire lifespan of the application. v8 parses that object at the time of parsing the document, regardless of where you put it. The real optimization would be to provide it as a string and parse it when first used. That would reduce main worker thread time when initializating the app. Such optimizations all they do is obscure the intention of the code with no real benefit to the end user. But have it your way. Pushed an update. |
@gajus it looks like you need to run prettier as the lint check is failing |
Done |
Great optimization @gajus. I've added it to the 3.2 milestone but we may need to actually delay this until Vite 4. There are projects and plugins out there that are parsing the current module preload lists to modify the paths using regex, because it wasn't possible to do configure how module preload worked in Vite. |
Work on Vite 4 has begun. It looks like there's a merge conflict in this PR that would need to be resolved |
No longer going to champion this, but someone else feel free to takeover. |
We ended up monkey patching node_modules (albeit with some difficulty) This reduced the entry script size from 539 692 bytes to 336 217 bytes, i.e. 40% reduction. But what is more surprising is that the total bundle size reduced from 3.3 MB to 2.5 MB, i.e. 24% reduction. |
@benmccann can I get your support to get this released to the public? What needs to be done? |
The merge conflict needs to be fixed. This would be a good time to do it if you'd like to get the change in since we're currently in beta for Vite 5 and patak had concerns about this potentially being a breaking change: #9491 (comment) |
Will re-open a new PR. Quite a few things changed upstream to continue on this branch. |
Description
This change introduces
__viteFile
helper which is a generated function that allows to refer to chunks by an index.Using this helper allows to generate import dependencies without repeatedly hard-coding the same strings.
On a medium size
vite-plugin-ssr
project, this reducesentry-entry-client-routing
file size from 1.7MB to 0.3MB.How it changes output
Before:
After:
File size impact
Before:
After:
The end result is a faster page loading time because of lesser file size and less time taken to parse the file.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).