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 rollup@4.53.0. You can test it via |
|
hey @lukastaegert, thank you so much for the merge and sorry for the reply! work on my side has been hectic. this literally shined my week and i can't believe my eyes. thank you so much for the merge. |
|
i'll hopefully write a blog post on thundraa.com, and if you think it'd be appropriate, i'd be happy to host it on the rollup blog/docs instead, let me know. |
|
@Aslemammad unfortunately your change broke one of our use cases. For a minor version bump this feels like a breaking change. |
PR rollup#5947 added "exports" to the universal RESERVED_NAMES set, which caused every variable or parameter named "exports" to be renamed to "exports$1" in any scope, even when no conflict exists with the output format's runtime "exports" identifier. Because eval strings are not rewritten, this silently broke code like: (unused, exports) => { console.log(exports.bar); eval('exports.bar = 1'); } where the parameter was renamed but the eval argument was not. The same over-aggressive rename also applied to unrelated nested bindings in ES output, which has no runtime "exports" at all. This change removes "exports" from RESERVED_NAMES and restores the system-format-specific check in ChildScope.addUsedOutsideNames so that nested scopes in SystemJS output still treat "exports" as used when they reference an outer exported binding. Top-level protection for CJS/AMD/UMD/IIFE/SYSTEM output is unchanged: RESERVED_USED_NAMES in Chunk.ts still seeds the chunk-level usedNames with "exports", and reassigned exports still carry renderBaseName === "exports" which propagates into nested scopes via getBaseVariableName(). The deconflictTopLevelVariables cache from PR rollup#5947 is untouched, so its performance benefits are preserved; a benchmark that reliably sees the PR's generate-time speedup (~20% vs pre-PR) shows no regression from this change. Fixes rollup#6357 Made-with: Cursor
…llup#6360) PR rollup#5947 added "exports" to the universal RESERVED_NAMES set, which caused every variable or parameter named "exports" to be renamed to "exports$1" in any scope, even when no conflict exists with the output format's runtime "exports" identifier. Because eval strings are not rewritten, this silently broke code like: (unused, exports) => { console.log(exports.bar); eval('exports.bar = 1'); } where the parameter was renamed but the eval argument was not. The same over-aggressive rename also applied to unrelated nested bindings in ES output, which has no runtime "exports" at all. This change removes "exports" from RESERVED_NAMES and restores the system-format-specific check in ChildScope.addUsedOutsideNames so that nested scopes in SystemJS output still treat "exports" as used when they reference an outer exported binding. Top-level protection for CJS/AMD/UMD/IIFE/SYSTEM output is unchanged: RESERVED_USED_NAMES in Chunk.ts still seeds the chunk-level usedNames with "exports", and reassigned exports still carry renderBaseName === "exports" which propagates into nested scopes via getBaseVariableName(). The deconflictTopLevelVariables cache from PR rollup#5947 is untouched, so its performance benefits are preserved; a benchmark that reliably sees the PR's generate-time speedup (~20% vs pre-PR) shows no regression from this change. Fixes rollup#6357 Made-with: Cursor Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>
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.