Class method effects #4018
Class method effects #4018
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4018 +/- ##
==========================================
+ Coverage 97.49% 98.00% +0.51%
==========================================
Files 193 201 +8
Lines 6824 6981 +157
Branches 2005 2049 +44
==========================================
+ Hits 6653 6842 +189
+ Misses 84 66 -18
+ Partials 87 73 -14
Continue to review full report at Codecov.
|
|
#4011 was merged. I'll tweak tests to improve coverage and push again. |
So that it sits in one place and is easier to extend.
This aims to improve tree-shaking of code that uses static class properties (#3989) and to improve detection of side effects through class getters/setters (#4016). The first part works by keeping a map of positively known static properties (methods and simple getters) in `ClassNode.staticPropertyMap`, along with a flag (`ClassNode.deoptimizedStatic`) that indicates that something happened that removed our confidence that we know anything about the class object. Access and calls to these known static properties are handled by routing the calls to `getLiteralValueAtPath`, `getReturnExpressionWhenCalledAtPath`, and `hasEffectsWhenCalledAtPath` to the known values in the properties. In contrast to `ObjectExpression`, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes. The handling of side effect detection through getters and setters is done by, _if_ the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized.
|
I was unable to create tests that run |
|
|
||
| const propertyMap = this.staticPropertyMap = Object.create(null); | ||
| const seen: {[name: string]: boolean} = Object.create(null); | ||
| for (const definition of this.body.body) { |
There was a problem hiding this comment.
Can this be done by building an up-front lookup table similar to how we do this for object expression? For classes with many definitions, this will scale badly as mayModifyThisWhenCalledAtPath is called during tree-shaking with potentially many passes (dozens in larger code bases) with potentially many calls per pass. This is a highly performance critical code path.
Maybe some code can be even extracted from/shared with object expression. The object expression logic is indeed very powerful as it will also pre-resolve computed properties if possible, handle quoted properties, handle multiple definitions of the same name and handle unknown computed properties that might overwrite previous properties.
There was a problem hiding this comment.
I decided against sharing code with ObjectExpression because there's a lot of small differences between how this works and how objects work (in syntax tree shape, handling of builtin properties, static/prototype distinction, and so on) and the helper functions would become monsters with a list of boolean parameters, and I prefer some vague duplication over that most of the time.
Building up a table is probably a good idea. Looking into that.
There was a problem hiding this comment.
Wait, the thing you're commenting on here is building up a table. The suggestions holds for mayHaveGetterSetterEffect, but, as far as I can see, only there.
src/ast/nodes/shared/ClassNode.ts
Outdated
| kind: 'get' | 'set', isStatic: boolean, name: string, | ||
| context: HasEffectsContext | ||
| ) { | ||
| for (const definition of body.body) { |
There was a problem hiding this comment.
Again this could benefit from a pre-built lookup table.
src/ast/nodes/shared/ClassNode.ts
Outdated
| } | ||
|
|
||
| deoptimizeCache() { | ||
| this.deoptimizeAllStatics(); |
There was a problem hiding this comment.
I do not believe this is necessary unless you rely on retrieving literal values for computed properties. That is also why you cannot test it.
So it works like this:
When you perform a getLiteralValueAtPath or a getReturnExpressionWhenCalledAtPath you pass a DeoptimizableEntity as the last parameter (which is usually the Node requesting the literal/return value). The reason is that it is possible a literal/return value might be deoptimized at a later time when binding/tree-shaking progresses. If that happens, then some Node down the chain that is responsible for the deoptimization will call deoptimizeCache on this entity. At this point in time, the entity is responsible for "forgetting" the cached value and also deoptimize the cache of all dependent entities that may rely on that value.
Admittedly it is kind of difficult to describe such deoptimization scenarios. Ideally I would just put a breakpoint into "deoptimizeCache" of ObjectExpression and see what test triggers it.
This will become even more important in the future as one goal is to move more and more of the initial deoptimizations to "on demand late deoptimization" i.e. only deoptimize when as reassignment is included.
There was a problem hiding this comment.
I do not believe this is necessary unless you rely on retrieving literal values for computed properties.
You mean this is only necessary for nodes that themselves retrieve literal values? Do I understand correctly that the call-throughs to definition nodes in the class' own getLiteralValueAtPath and getReturnExpressionWhenCalledAtPath don't count because they just pass through their argument instead of passing themselves?
|
Attached patches drop the |
|
Rebased onto current master branch and added the new argument to |
I'm open to that, but there's so much going on there that I couldn't figure out what the system used there works like. If you could do a quick rundown of that logic, I'll take a look. |
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install marijnh/rollup#class-method-effectsor load it into the REPL: |
|
As I often find myself looking up the logic again, I pushed a comment explaining the logic to As a matter of fact, I just discovered that the implementation has a bug because apparently var obj = {
foo: 'value',
set foo(value) {}
}
console.log(obj.foo);will not log |
|
Thanks for those clarifications. Unfortunately, I still find the And since I also found out that, even with the extensions proposed in this PR, Rollup still won't remove the calls that motivated this work (due to dereferencing an object parameter), I'm not really motivated to continue on this. Feel free to close it or to keep it around for future reference. Sorry for wasting your time. (As a general—probably quite unhelpful—observation, I think the library would benefit from a more rigorous abstract interpretation framework, as the current approach, which I assume grew organically as new requirements come up, doesn't really seem to be up to the complicated uses that are being made of it. But of course that kind of overhaul would be a huge amount of work, and who has time for that.) |
|
Yes I understand. Please leave this PR open, I will try to take it over in the next days.
Probably, and there will be larger refactorings in the future as there have been in the past. However for the sake of continuous improvement in smaller steps, one takeaway from this story would be that we would benefit from an abstract "JavaScript object" data type/class/... where nodes like |
|
Breaking with the rule of "keeping a PR minimal and to the point", I have abused this PR now to also fix quite a few issues in Rollup around the use of getters and setters. I also released a pre-release version you can try via This is now ready for release, but I will keep it open for a few days before doing so to let people give it a spin. Core ChangesAfter being taken over, this PR has been very much extended beyond the original scope. Under the hood, two core changes have been introduced:
New Features
|
|
Thanks for continuing this after I gave up. The changes sound like they address a number of the pain points I ran into. I tried bundling a bunch of my projects with the beta and ran their tests without seeing any regressions. |
|
One improvement I forgot to list is that now all deoptimization happens lazily, i.e. all assignments in dead code will no longer mess up the analysis. Before, these kinds of deoptimizations were happening during the "bind" phase, which was causing its own bunch of issues. Now, the "bind" phase is solely for binding variables to nodes as it was originally intended 😉 |
|
Simplified test case: let a = [];
a.push("pass");
console.log(a[0] || "fail");console.log("fail"); |
|
Thanks a lot, great catch! There was a bug in the array deoptimization tracking that did not consider mutations via builtins in one of the places. Fixed now. Also released another beta. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Resolves #3989
Resolves #3250
Resolves #4016
Updated Description
To try out this PR before it is released, use
npm install rollup@beta.Core Changes
After being taken over, this PR has been very much extended beyond the original scope. Under the hood, two core changes have been introduced:
ExpressionEntitythat follows a "conservative by default" approach. I.e. a node that does not implement any methods of its own will behave identical to an "unknown expression" meaning any call, assignment or even access to this node is a side-effect and we treat it as an "unknown value" in conditionals.ObjectEntitythat implements object-like behaviour with regard to property access that is used by object literals, array literals, classes and virtual object and arrays generated by builtin methods. In the future, I hope to extend this to functions as well. This means that improving the behaviour of objects means just working on the ObjectEntity.New Features
ObjectEntityUpdated REPL, v2.48.0 REPLPossible extensions for future PRs:
ObjectEntityfor functions and arrow functions. For now, those will deoptimize completely when properties are assigned.__proto__and uses ofObject.setPrototypeOf/Object.getPrototypeOf. In some situations, mutations may still not be detected here. This is a tricky one as technically, passing an object to an unknown method could mutate the object prototype. We will need to think what assumptions we keep in the system, e.g. "builtins always behave as expected". Maybe we add a flag here.Original Description (outdated)
Track static class fields and improve handling of class getters/setters
This aims to improve tree-shaking of code that uses static class properties (#3989) and to improve detection of side effects through class getters/setters (#4016).
The first part works by keeping a map of positively known static properties (methods and simple getters) in
ClassNode.staticPropertyMap, along with a flag (ClassNode.deoptimizedStatic) that indicates that something happened that removed our confidence that we know anything about the class object.Access and calls to these known static properties are handled by routing the calls to
getLiteralValueAtPath,getReturnExpressionWhenCalledAtPath, andhasEffectsWhenCalledAtPathto the known values in the properties. In contrast toObjectExpression, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes.The handling of side effect detection through getters and setters is done by, if the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized.