[WIP] Rewrite #902
[WIP] Rewrite #902
Conversation
| "plugin:import/warnings" | ||
| ], | ||
| "parserOptions": { | ||
| "ecmaVersion": 6, |
There was a problem hiding this comment.
yeah, will change that
|
What about caching? With buble this way is ok because we have incremental build. But rollup will process all hundreds files. I can try home to compare branches. |
Right now, ASTs aren't reused between runs, meaning Acorn has to re-parse each module. That should be fixable, and once it is I'd expect the caching to have the same impact on incremental builds as before. |
|
We need to update acorn |
|
Should be fine with the last version |
|
Hm.. acorn need to release... So long. |
|
Let's enable classes transpilling for now. Do same **** for angular2 because they like decorators which requires reflection polyfill which requires transpilling because of the same "do not call class". |
|
And now I got Is that a bug in node or somthing else? |
|
I don't quite follow the
Hmm, possibly, what Node version are you on? A |
|
v6.3.1 We can't invoke classes without |
|
Enabled transpilling only for acorn. It works! Finally. Now two times slower. |
|
Weird, was working fine for me.
what is two times slower? |
|
As you said ast is not cached yet. This is the result. I'm checking on that big project. |
|
I think we should fix it before merge to not make rollup worse. |
|
Ah, and I mean incremental build. Not regular. |
Agree. Done 83ccb97 |
src/ast/nodes/VariableDeclaration.js
Outdated
| let shouldSeparate = false; | ||
| let separator; | ||
|
|
||
| if ( this.scope.isModuleScope && !/forStatement/.test( this.parent.type ) ) { |
There was a problem hiding this comment.
Is this supposed to be ForStatement?
There was a problem hiding this comment.
ugh, actually it was supposed to be
const forStatement = /^For(?:Of|In)?Statement/;
// ...
`!forStatement.test(...)good catch! will fix
|
This branch seems to work with decaffeinate and its various projects. However, it does fail to treeshake a helper added by babel, the function set() {
set();
}That function is preserved despite its only reference being inside itself. |
ah, of course – if a function calls itself Rollup assumes the worse, and it doesn't actually need to. Fix inbound |
|
I just tried to verify that your fix caused // index.js
var set = function set(object, property, value, receiver) {
var desc = Object.getOwnPropertyDescriptor(object, property);
if (desc === undefined) {
var parent = Object.getPrototypeOf(object);
if (parent !== null) {
set(parent, property, value, receiver);
}
} else if ("value" in desc && desc.writable) {
desc.value = value;
} else {
var setter = desc.set;
if (setter !== undefined) {
setter.call(receiver, value);
}
}
return value;
};It turns out it's trying to enhance the Node {
type: 'Literal',
start: 294,
end: 301,
value: 'value',
raw: '"value"' } |
Ah, I know why that is. It doesn't have a lookup table of child nodes for each node type, instead it builds that table as it encounters each node – so the first time it encounters a If the first As to why |
|
Benchmark |
src/Module.js
Outdated
| this.statements = this.parse(); | ||
|
|
||
| this.declarations = blank(); | ||
| this.type === 'Module'; // TODO only necessary so that Scope knows this should be treated as a function scope... messy |
There was a problem hiding this comment.
Should this really be ===?
There was a problem hiding this comment.
If you're strong type checking then 👍 @Victorystick. Or maybe i'm missing the context.
There was a problem hiding this comment.
Ha, yeah – should be this.type = 'Module'. Fixed
|
Have released this as 0.35 so that we have a solid base from which to start optimising and addressing any lingering bugs.
I think I have an understanding of why this is happening now. It's because the function expression references both itself and a function with an unknown value ( The fix I envisage is something I was considering separately as a performance optimisation – only enhancing and initialising nodes as necessary. Ideally, as far as Rollup is concerned, in the final tree-shaking stage that declaration should look like this: var set = function set ( /* who cares */ ) {
/* who cares */
};Unless Thanks, that's useful. How are you running those numbers? Wonder if it's something we should have in the library all the time (perhaps behind |
|
@Rich-Harris I use this, require it and add points every time when code is changed. |
Over the weekend I got a chance to scratch an itch I've had for a while. There are some surprising bugs with the current version of Rollup, such as #901, that are rather difficult to fix with the existing codebase. On top of that, our tree-shaking is too coarse (it only operates on top-level statements/declarations) and too conservative (it sees potential effects where there are none).
This PR intends to lay the foundations for much more sophisticated static analysis, which will allow us to fix those bugs (I just checked – it fixes #901) and eventually do a much better job of tree-shaking libraries like Lodash, which have a lot of statements that look like they have effects but don't.
It's basically a complete rewrite – sorry – so I'll try to give a high-level overview. Previously, each
Bundlewas composed of severalModuleobjects, each of which had an array ofStatementobjects – top-level statements and declarations. Now, there's noStatement– instead, we analyse the AST nodes themselves.In order to do that, the AST nodes are 'enhanced' with various properties and methods. Each node's
initialisemethod is called, causing declarations (variable, function or class) to register themselves with the current scope. (Each node is responsible for initialising its children, which means that for example a'x' !== 'x' ? a() : b()expression can stopa()being initialised.) Later, webindeach node, such that each reference is linked to the declaration it references.The final step before rendering is to
runeach node that has effects. (An 'effect' means that it impacts the outside world in some way – e.g.a = 1only has an effect ifais exported or used elsewhere, butwindow.a = 1always has an effect.) This means walking over the AST and marking statements so that they get included in the final bundle. If we walk over anIdentifierthat references a declaration, we 'activate' the declaration – so if we encounterfoo()and Rollup judges that thefoofunction has effects (if it's unsure, it errs on the side of caution), bothfoo()andfunction foo () {...}will be included.Key to this is the idea that we can track the values of nodes. Because we know that
fooreferences a function declaration, it's easy to know what code gets run whenfoo()happens. If instead we hadvar foo; foo = condition ? a : b, we know that there are two possible values forfoo– if eithera()orb()would have effects, so doesfoo(). Iffoowas a global or something imported from an external module, Rollup would assume it has effects.This all turns out to be less hairy than you might imagine, though I'll readily confess that, as with most large rewrites, the purity of the original idea isn't quite reflected in the implementation. There are some rough edges, some unnecessarily verbose code, and doubtless some bugs. It isn't quite ready for merge just yet – need to test the performance, try it out with some more real-world code, etc (plus there are two failing tests related to detecting when cyclical dependencies are in the wrong order), but if @rollup/collaborators are on board then I'd like to merge it before too long otherwise it will block all other work.
Next steps
Assuming there are no showstoppers here – and I'm reasonably confident with this general approach – phase 2 of the operation will be to add more value-tracking logic. For example, this...
...doesn't have an effect, but because Rollup sees the call expression and doesn't know that
String.prototype.replaceis effect-free, it includes both lines in the output regardless of whetherstringis used. And in this case......it should be possible to remove the
bmethod if Rollup understands a) properties and b) classes/prototypes.