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
Improve circular dependency execution order #3840
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via
or load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #3840 +/- ##
==========================================
+ Coverage 97.08% 97.13% +0.04%
==========================================
Files 187 188 +1
Lines 6563 6669 +106
Branches 1913 1940 +27
==========================================
+ Hits 6372 6478 +106
Misses 101 101
Partials 90 90
Continue to review full report at Codecov.
|
I'll try this branch for our project with really messed circulars 👍 |
Also have a look if you notice any serious performance deterioration. |
Run on project with 2412 total resulting chunks. js & js_commonjs-proxy only calculated, plus assets. There is postcss, babel, commonjs, node-resolve plugins and more. Treeshaking is disabled, but it not changes time significantly. rollup@2.32.1 rollup@rollup/rollup#improve-execution-order So, performance is not changed, but imports is not working too :) There is still circular import with TDZ error, but now it's not very easy to reason about... I trying to extract it into example. |
Ok, first of all, with disabled and enabled tree shaking, error thrown in different files. I think it should be not so! @lukastaegert have you a thoughts why so is happens? |
Ok, there is repro: https://github.com/Amareis/rollup-bug-repro |
I see that current implementation isn't preserving exact modules exports, even with tree shaking disabled: in repro app is reexporting everything from models/index, which reexporting all from models/internal, but in resulting app.js there is only reexports from models/data... |
re-exports are shortened if possible. But this should not affect execution order. Or to be more precise, side-effect order. So the current implementation tries to maintain execution order between all modules that have side-effects. So e.g. a module that just contains re-exports but no side-effects will vanish, but it could happen that additional side-effect-only imports are generated to keep the side-effect order. It should also maintain execution order between modules that declare a variable and modules that consume a variable, but maybe the implementation still has issues there. |
In any case, if you can whip something up that runs in Node but produces TDZ errors after bundling, that would be great. |
Actually, even example from #3834 still broken :) |
Yes, chunk execution ordering has always implicitly been based on the assumption that circular references are contained in the same chunk. This seemed a safe assumption because there is no network loading scenario where you wouldn't want this. With manual chunks and preserve modules, it is possible to break this invariant. Ideally we could catch that case and patch it up somehow, but if circular references between chunks is desired we can certainly make this work though of course. The important thing to note firstly though is that execution order is entirely dictated by the entry points. A cycle will execute in different orders depending on which entry module is imported first (and therefore which module in the cycle is imported first). Thus you cannot determine execution order if you do not take into account entry points. My concern is that this PR may be missing that detail. I'd be very careful changing this algorithm though and would need some time to more carefully verify the post-order execution order invariants from the spec are maintained under this new algorithm to finish review. |
I have been very aware of this problem for a long time and assure you this PR does not miss this fact, but it also does not fix it. While the original implementation completely missed that fact as everything was based on the execution order of the first entry point, this PR actually improves this situation rather than making it worse because instead of relying on this execution order, it treats every single file as an entry point and does a kind of topological sorting of external dependencies separately for each single file. But this aside, I finally understand the original issue which was indeed the re-export shortening logic. In short, we just cannot shorten imports in a preserveModules build if this build contains cycles. I am currently not sure how true is this in general or if it is only a problem if we import from a file that is kindof involved in the cycle. For regular chunking builds, we do not have this problem yet but instead the other problem that you mention. But this is not the issue here, and the goal is not to solve this problem with this PR. This will be solved at another time. Going back to the issue at hand, I think my current best solution would be
|
Right, ideally the execution order would be computed for each entry point, and then it would be ensured that the order invariants of that multi execution profile would be maintained under any transformation. Right now, we do overclassify from the first entry point causing stricter ordering invariants than might be optimal. Although I suspect this approach will be causing looser invariants against the invariants I describe above. Is your suggestion to disable reexport shortening for all builds? Or only preserve modules? Perhaps preserve modules should get its own output algorithm entirely and not use the Chunk graphing system. |
I do not think we overclassify the order as we already taken side-effects into account, optimizing and removing dependencies where possible. The goal here really is not module order but side-effect order. But yes, my suggestion is for now to use a different algorithm when preserving modules, and think about a new algorithm for the chunking only when we fix the other execution order problem there. Because when chunking, the solution would be to create more chunks, and then it is no longer guaranteed that all cycles are within a single chunk. |
This is a great point and leads to a nicer generalization yes. See also how esbuild constructs its chunks if you haven't read it already (https://github.com/evanw/esbuild/blob/master/docs/architecture.md#code-splitting). Although I haven't verified that approach either TBH. Because of TDZ circular references can effectively be considered to require side effect ordering. With regards to the TDZ discussion I do again feel that assuming TDZ isn't hit is a worthy assumption because there is no valid program that wouldn't require that assumption. Like all cycles being contained in chunks, these are assumptions where there is no valid use case for the alternative, so engineering effort to support something that doesn't have a use case, when it could just be worked around doesn't sound like the best use of our scarcest resource to me! Edit: I mean TDZ for the top-level execution specifically. |
(Re)export signatures should be preserved if treeshaking is explicitly disabled in conjuction with |
Well, the (re-)export signature is preserved, it is just not preserving where the reexports are coming from... 🤪 |
So, I managed how to preserve all exports - just emit additional entry chunk for every module, which has only reexports from module. It's still needed for synthetic name exports to wokring, so it's ok. But this issue is still show-stopper :) Can we make some wokaround? For example just don't shortening urls from circular chains, it seems to possible even in "old" implementation. |
I will try to find some time to invest in this soon, but other things came up. |
Problem is, this will not be a simple fix as importing a variable from a file where it was not declared is not easily representable with the current data model where each variable has a single module assigned. So what we need is to represent that module |
What about new option, Then, this mode can be used for circular dependencies in simple mode. |
Not sure what the advantage of this tool over Babel would be, then. No, I am quite sure this can be solved in a better way, but for now I would like to finish work on rollup/plugins#537 first as this has been open for months now. |
Just to outline my thoughts so far:
|
All of rollup plugins, of course! Plus, seamless switch between development and production builds, since resolve logic stay the same and all of emitted chunks/assets too. In this mode rollup becames great development tool, and it's even not-so-hard to maintain, since basically it's just full deoptimisation, just like debug build. |
7d42c4d
to
e2ee8ae
Compare
e2ee8ae
to
b11a14c
Compare
Nope, there is still same error and imports in resulting files is still shortened. Maybe it worth to add original example as a test? |
Ah sorry, yes, wrong PR. This problem is still not fixed, will be soon. |
After spending basically weeks of my limited time on trying to improve the situation here, I decided to not solve the remaining issue in this PR. The answer is:
Note that the issue is solely an issue for To illustrate, take this example: // main.js
export { getSecond } from './first.js';
export { second } from './second.js';
console.log('main');
// first.js
import { second } from './main.js';
export const getSecond = () => second;
console.log('first');
// second.js
import { getSecond } from './main.js';
export const second = 'second';
console.log('second:', getSecond()); The correct output would be
Now if we were to naively transpile this to AMD file-by-file, we would get // main.js
define(['exports', './first', './second.js'], function (exports, first, second) {
console.log('main');
exports.getSecond = first.getSecond;
exports.second = second.second;
});
// first.js
define(['exports', './main'], function (exports, main) {
console.log('first');
exports.getSecond = () => main.second;
});
// second.js
define(['exports', './main'], function (exports, main) {
console.log('second:', main.getSecond());
exports.second = 'second';
}); This could will just crash with "undefined is not a function" when second.js tries to call For CommonJS, the problem is theoretically fixable by making sure the reexports are not assigned at the bottom of the file but directly after the corresponding imports, i.e. // main.js
var first = require('./first');
exports.getSecond = first.getSecond;
var second = require('./second');
exports.second = second.second; However to make this logic work in all instances, we would need a major rewrite for how star reexports are handled to ensure proper precedence of exports and duplicate a lot of logic as AMD and CJS would now diverge and could no longer share the export logic. All in all, it is a major investment that I currently do not see as returning the same benefits as if I were to work on other things as it may likely block all other Rollup progress for another month (especially with a lot of other personal issues I have to deal with mainly concerning child-care in a pandemic). So I will see if I can add a warning but otherwise release this PR to unblock progress on other things. |
I think systemjs should works in such cases? It was developed to reflect esm semantics. |
Not circular dependencies, at least not this aspect. This only works for ESM because all modules are parsed and analysed in advance, resolving all imports and exports first, so that an export |
For systemjs there is solution which is one-to-one to provided commonjs solution. At least my project is working good when I transpiled all my modules to systemjs directly, without any imports shortening. Whole point of systemjs is to fully reflect esm semantics, isn't it? |
I extended the plugin to add a proper warning for now. It should be possible to add proper logic in the future for reexports based on this, but it will still be a lot of work to do properly, especially with regard to how external namespace reexports should play into this. One approach could also be to extend this gradually format by format, starting with ESM and showing the warning only for other formats, and then possibly adding SystemJS if it really works as intended. My intention is to release this soon as the CommonJS plugin will rely on some part of this. Also have a look at the updated description. |
Ok, so currently it still not working for my project with preserveModules, but its warns a ton. And production build (without preserveModules) working, but takes a much longer, 3 minutes vs 2 minutes before it. |
I wonder where the slowness comes from. Is it the same project you tested here: #3840 (comment) ? If not, how does performance change for that project? |
If the slowness did not appear with the version you tested in October (which you can still try via
|
Oh, my bad. I checked this branch, rollup 2.35.1 and rollup 2.33.1, there is always about 4 minutes for production build (with terser and sourcemaps). I think in october I ran builds without minification and sourcemaps, that's why additional minutes added! |
Ah, thanks for checking. I was really worried about a bad performance debugging session. |
@lukastaegert see #3801 (comment) regarding the comment about support reexports in systemjs. It's not possible yet with rollup, but can be made to support it. |
Sounds good. Hope to find some time soon to pick this up again so that we can have this at least for ESM and SystemJS, and possibly CommonJS. |
e8fdc97
to
bc5a186
Compare
bc5a186
to
b652137
Compare
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
#3834
Description
As #3834 showed, our current way of sorting dependencies in chunks completely messes up execution order if there are circular dependencies between chunks, which can happen when preserving modules.
Before, we just sorted dependencies by their execution order. This PR instead changes the logic so that the order of dependencies is preserved from the modules. If we skip dependencies because they have no side-effects, the algorithm crawls transitive dependencies in the same order as regular dependency execution would happen to look for transitive side-effect dependencies instead.
When merging module dependencies into chunks, the process now has two parts
This new algorithm will produce better execution order when
The second is not enforced yet, though. This will be a larger feature to add sometime in the future.
Unfortunately, the execution order is not perfect when preserving modules as import shortening will still introduce new imports into modules/chunks that can trigger a cycle to be executed in a non-natural order. For this case, there has been a warning added. The infrastructure introduced to generate this warning can serve as a basis to fix this issue at least for ESM, CommonJS and possibly SystemJS in the future. AMD output cannot be made to support this easily as it does not have a dedicated concept of reexports. For CommonJS, the solution must be to hoist reexports from a module directly below the corresponding require statement in the reexporting module, i.e.
should become
However, special care needs to be taken to properly support namespace reexports mixed with regular (re)exports as those will always override namespace reexports. Also, the logic should look different for reassigned exports.
Another point that was added in this PR that is not directly related to circular reexports is which reexporting modules are scanned for side-effects when moduleSideEffects are turned off. Basically the logic now goes like this:
So in short, a direct reexport (export ... from) is ignored when looking for side effects while a reexport using separate import/export statements is considered. The reasoning here is that when I import a binding and the export it again, I may be my intention to mutate this binding in between, e.g. add properties to an object. The commonjs plugin will rely on this feature.