refactor: store safe variable names in cache for subsequent usage #5947
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
lukastaegert
left a comment
There was a problem hiding this comment.
Hi, thanks for the interesting impulse. This is definitely something to consider looking into. Nevertheless, the current approach has some issues. I added a test to demonstrate the most severe one: When a module is changed in a way that would produce a name conflict, the names in the updated module are no longer deconflicted, resulting in an invalid bundle.
In general, we have an issue with reproducibility: With this change, the result no longer depends on the input alone but also on the name cache. If top level variables change in one module (or even just the import order), a fresh build might deconflict differently than a build using the cache. I wonder if it is possible to work around this in some way.
|
hey! thank you so much for the review. i'll look into this and try to resolve it. is it just the test case need to be resolved, or we have to inspect the whole general issue? if it's the latter, i'd appreciate some pointers that i should look into since my knowledge of the codebase is pretty limited. but anyway, i'll try to figure this out soon. |
|
I think it is a more general issue, unfortunately I do not see a simple solution yet. Another problem is if the accessed global variables change in a module, which can influence deconflicting in all other modules. Maybe one needs to figure out a performant way to check if the accessed top-level and global symbols changed in a module. |
|
@lukastaegert hey! i just fixed the tests. it was because of a special case where the name was I also resolved the order issue we talked about earlier by only adding to usedNames iteratively rather than all in once. |
|
sorry i just closed this by misclicking the close with comment button. re-opened now! |
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install Aslemammad/rollup#feat/safeVariableNamesNotice: Ensure you have installed the latest nightly Rust toolchain. If you haven't installed it yet, please see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust. or load it into the REPL: |
Performance report
|
acac6b6 to
e3733c0
Compare
lukastaegert
left a comment
There was a problem hiding this comment.
Ok, from my side I am really happy now with how this turned out. And I am really sorry for the extremely long wait. There were many other things going on on my side and maybe you understand that I was somewhat afraid of this PR and its consequences. However, I think this solution is pretty solid and also future-proof now. Also, the performance report already shows how this speeds up bundling Rollup itself, which is amazing.
Please have another look. If you are fine and have no more comments, we can release this now.
|
This PR has been released as part of [email protected]. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
While testing the 10000 vite benchmark example I found out the main bottleneck was the
deconflictTopLevelVariablesfunction (avoids using the same name for variables when all the chunks get merged together). Even after introducing thecacheproperty which required few changes on the vite side, still that caused a small performance improvement (1s-2s).After digging in, I came to find that the cache was only being used in the bundle generation step and not in the
generateorwritestep wheredeconflictTopLevelVariables->getSafeNamesits.The idea here is to reduce the usage of
getSafeNameby caching the already generated safe names by the same function. The new cache field is simply namedsafeVariableNameswhich is an object. This results in a 2x performance improvement (12s -> 5s).Teams can achieve a way better build performance specially in CI/CDs (e.g. deploys) once they start using the
cacheproperty on rollup.