Skip to content
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

Class method effects #4018

Merged
merged 56 commits into from May 23, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
7b8304e
Move logic from ClassBody into ClassNode
marijnh Mar 25, 2021
d524abd
Track static class fields and improve handling of class getters/setters
marijnh Mar 25, 2021
185097d
Remove ClassNode.deoptimizeCache
marijnh Mar 27, 2021
0960a33
Keep a table for class property effects
marijnh Mar 27, 2021
cea520d
Add comment explaining property map
lukastaegert Apr 2, 2021
bcd7f5a
Merge branch 'master' into class-method-effects
lukastaegert Apr 14, 2021
0e0fdaa
Merge branch 'master' into class-method-effects
lukastaegert Apr 14, 2021
1251f5a
Fix types
lukastaegert Apr 15, 2021
d8ec2ea
Merge branch 'master' into class-method-effects
lukastaegert Apr 15, 2021
056d9f0
Make getReturnExpression and getLiteralValue more similar for objects
lukastaegert Apr 17, 2021
9507110
Use common logic for return expression and literal value
lukastaegert Apr 17, 2021
7f51eef
Use common logic for return access and call effects
lukastaegert Apr 17, 2021
10c4e40
Extract shared logic from ObjectExpression
lukastaegert Apr 19, 2021
99a794d
Use an object for better performance
lukastaegert Apr 19, 2021
72dc62b
Simplify handling for setters and other properties
lukastaegert Apr 20, 2021
ba6e043
Small simplification
lukastaegert Apr 20, 2021
f19fc61
Work towards better class handling
lukastaegert Apr 21, 2021
132fb94
merge ObjectPathHandler into ObjectEntity
lukastaegert Apr 21, 2021
f09cd5b
Slightly refactor default values
lukastaegert Apr 21, 2021
10ee697
Separate unknown nodes from other Nodes to avoid future circular depe…
lukastaegert Apr 21, 2021
0350110
Introduce new prototype tracking
lukastaegert Apr 22, 2021
d723df6
Improve coverage
lukastaegert Apr 23, 2021
a2f02c1
Fix class deoptimization in arrow functions via this/super
lukastaegert Apr 23, 2021
f89085c
Simplify and merge property and method definition
lukastaegert Apr 23, 2021
6d70426
Improve coverage
lukastaegert Apr 23, 2021
4eff4ed
Replace deoptimizeProperties by deoptimizing a double unknown path
lukastaegert Apr 24, 2021
dd5f73b
Assume functions can add getters to parameters
lukastaegert Apr 24, 2021
333116a
Improve class.prototype handling
lukastaegert Apr 25, 2021
8f498f2
Assume created instance getters have side-effects
lukastaegert Apr 25, 2021
ef0929f
Base all expressions on a base class
lukastaegert Apr 25, 2021
0fcd42d
Merge branch 'master' into class-method-effects
lukastaegert May 6, 2021
145d01f
Only deoptimize necessary paths when deoptimizing "this"
lukastaegert Apr 30, 2021
71a27ba
Handle deoptimizing "this" in getters
lukastaegert May 6, 2021
616a6d3
Handle deoptimizing "this" in setters
lukastaegert May 8, 2021
182a0ba
Merge branch 'master' into class-method-effects
lukastaegert May 8, 2021
e33e06e
Unify this deoptimization
lukastaegert May 8, 2021
afad828
Simplify recursion tracking
lukastaegert May 9, 2021
d290c6c
Get rid of deoptimizations during bind phase
lukastaegert May 10, 2021
0609907
Get rid of unneeded double-binding checks
lukastaegert May 10, 2021
a70e660
Inline deoptimizations into NodeBase for simple cases
lukastaegert May 10, 2021
ea5fe1d
Add more efficient way to create object entities
lukastaegert May 10, 2021
02c1127
Add thisParameter to CallOptions
lukastaegert May 12, 2021
ab15b60
Move NodeEvents to separate file
lukastaegert May 12, 2021
ece8f31
Track array elements
lukastaegert May 14, 2021
275946e
Merge branch 'master' into class-method-effects
lukastaegert May 15, 2021
f4766a6
Simplify namespace handling
lukastaegert May 15, 2021
92b1e1d
Use Object.values and Object.entries instead of Object.keys where useful
lukastaegert May 15, 2021
224a4e1
Improve code and simplify literal handling
lukastaegert May 16, 2021
87e8029
Improve coverage
lukastaegert May 16, 2021
d2b61c9
Improve coverage
lukastaegert May 16, 2021
66a5cf9
Improve coverage in conditional and logical expressions
lukastaegert May 16, 2021
76b5cbe
Improve coverage
lukastaegert May 17, 2021
b6bd8d4
2.49.0-0
lukastaegert May 18, 2021
99964aa
Fix test to support pre-release versions
lukastaegert May 18, 2021
84b0d94
Fix failed deoptimization of array props
lukastaegert May 20, 2021
7490a87
2.49.0-1
lukastaegert May 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 0 additions & 27 deletions src/ast/nodes/ClassBody.ts
@@ -1,8 +1,5 @@
import { CallOptions } from '../CallOptions';
import { HasEffectsContext } from '../ExecutionContext';
import ClassBodyScope from '../scopes/ClassBodyScope';
import Scope from '../scopes/Scope';
import { EMPTY_PATH, ObjectPath } from '../utils/PathTracker';
import MethodDefinition from './MethodDefinition';
import * as NodeType from './NodeType';
import PropertyDefinition from './PropertyDefinition';
Expand All @@ -12,31 +9,7 @@ export default class ClassBody extends NodeBase {
body!: (MethodDefinition | PropertyDefinition)[];
type!: NodeType.tClassBody;

private classConstructor!: MethodDefinition | null;

createScope(parentScope: Scope) {
this.scope = new ClassBodyScope(parentScope);
}

hasEffectsWhenCalledAtPath(
path: ObjectPath,
callOptions: CallOptions,
context: HasEffectsContext
) {
if (path.length > 0) return true;
return (
this.classConstructor !== null &&
this.classConstructor.hasEffectsWhenCalledAtPath(EMPTY_PATH, callOptions, context)
);
}

initialise() {
for (const method of this.body) {
if (method instanceof MethodDefinition && method.kind === 'constructor') {
this.classConstructor = method;
return;
}
}
this.classConstructor = null;
}
}
221 changes: 208 additions & 13 deletions src/ast/nodes/shared/ClassNode.ts
@@ -1,47 +1,242 @@
import { getOrCreate } from '../../../utils/getOrCreate';
import { CallOptions } from '../../CallOptions';
import { DeoptimizableEntity } from '../../DeoptimizableEntity';
import { HasEffectsContext } from '../../ExecutionContext';
import ChildScope from '../../scopes/ChildScope';
import Scope from '../../scopes/Scope';
import { ObjectPath } from '../../utils/PathTracker';
import { EMPTY_PATH, ObjectPath, PathTracker } from '../../utils/PathTracker';
import { LiteralValueOrUnknown, UnknownValue, UNKNOWN_EXPRESSION } from '../../values';
import ClassBody from '../ClassBody';
import Identifier from '../Identifier';
import MethodDefinition from '../MethodDefinition';
import { ExpressionEntity } from './Expression';
import { ExpressionNode, NodeBase } from './Node';

export default class ClassNode extends NodeBase {
body!: ClassBody;
id!: Identifier | null;
superClass!: ExpressionNode | null;
private classConstructor!: MethodDefinition | null;
private deoptimizedPrototype = false;
private deoptimizedStatic = false;
// Collect deoptimization information if we can resolve a property access, by property name
private expressionsToBeDeoptimized = new Map<string, DeoptimizableEntity[]>();
// Known, simple, non-deoptimized static properties are kept in here. They are removed when deoptimized.
private staticPropertyMap: {[name: string]: ExpressionNode} | null = null;

bind() {
super.bind();
}

createScope(parentScope: Scope) {
this.scope = new ChildScope(parentScope);
}

hasEffectsWhenAccessedAtPath(path: ObjectPath) {
if (path.length <= 1) return false;
return path.length > 2 || path[0] !== 'prototype';
deoptimizeAllStatics() {
for (const name in this.staticPropertyMap) {
this.deoptimizeStatic(name);
}
this.deoptimizedStatic = this.deoptimizedPrototype = true;
}

deoptimizeCache() {
this.deoptimizeAllStatics();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

deoptimizePath(path: ObjectPath) {
const propertyMap = this.getStaticPropertyMap();
const key = path[0];
const definition = typeof key === 'string' && propertyMap[key];
if (path.length === 1) {
if (definition) {
this.deoptimizeStatic(key as string);
} else if (typeof key !== 'string') {
this.deoptimizeAllStatics();
}
} else if (key === 'prototype' && typeof path[1] !== 'string') {
this.deoptimizedPrototype = true;
} else if (path.length > 1 && definition) {
definition.deoptimizePath(path.slice(1));
}
}

deoptimizeStatic(name: string) {
delete this.staticPropertyMap![name];
const deoptEntities = this.expressionsToBeDeoptimized.get(name);
if (deoptEntities) {
for (const entity of deoptEntities) {
entity.deoptimizeCache();
}
}
}

getLiteralValueAtPath(
path: ObjectPath,
recursionTracker: PathTracker,
origin: DeoptimizableEntity
): LiteralValueOrUnknown {
const key = path[0];
const definition = typeof key === 'string' && this.getStaticPropertyMap()[key];
if (path.length === 0 || !definition ||
(key === 'prototype' ? this.deoptimizedPrototype : this.deoptimizedStatic)) {
return UnknownValue;
}

getOrCreate(this.expressionsToBeDeoptimized, key, () => []).push(origin);
return definition.getLiteralValueAtPath(
path.slice(1),
recursionTracker,
origin
);
}

getReturnExpressionWhenCalledAtPath(
path: ObjectPath,
recursionTracker: PathTracker,
origin: DeoptimizableEntity
): ExpressionEntity {
const key = path[0];
const definition = typeof key === 'string' && this.getStaticPropertyMap()[key];

if (path.length === 0 || !definition ||
(key === 'prototype' ? this.deoptimizedPrototype : this.deoptimizedStatic)) {
return UNKNOWN_EXPRESSION;
}

getOrCreate(this.expressionsToBeDeoptimized, key, () => []).push(origin);
return definition.getReturnExpressionWhenCalledAtPath(
path.slice(1),
recursionTracker,
origin
);
}

hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext) {
if (path.length === 0) return false;
if (this.deoptimizedStatic) return true;
if (this.superClass && this.superClass.hasEffectsWhenAccessedAtPath(path, context)) return true;
const key = path[0];
if (key === 'prototype') {
if (path.length === 1) return false;
if (this.deoptimizedPrototype) return true;
const key2 = path[1];
if (path.length === 2 && typeof key2 === 'string') {
return mayHaveGetterSetterEffect(this.body, 'get', false, key2, context);
}
return true;
} else if (typeof key === 'string' && path.length === 1) {
return mayHaveGetterSetterEffect(this.body, 'get', true, key, context);
} else {
return true;
}
}

hasEffectsWhenAssignedAtPath(path: ObjectPath) {
if (path.length <= 1) return false;
return path.length > 2 || path[0] !== 'prototype';
hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext) {
if (this.deoptimizedStatic) return true;
if (this.superClass && this.superClass.hasEffectsWhenAssignedAtPath(path, context)) return true;
const key = path[0];
if (key === 'prototype') {
if (path.length === 1) return false;
if (this.deoptimizedPrototype) return true;
const key2 = path[1];
if (path.length === 2 && typeof key2 === 'string') {
return mayHaveGetterSetterEffect(this.body, 'set', false, key2, context);
}
return true;
} else if (typeof key === 'string' && path.length === 1) {
return mayHaveGetterSetterEffect(this.body, 'set', true, key, context);
} else {
return true;
}
}

hasEffectsWhenCalledAtPath(
path: ObjectPath,
callOptions: CallOptions,
context: HasEffectsContext
) {
if (!callOptions.withNew) return true;
return (
this.body.hasEffectsWhenCalledAtPath(path, callOptions, context) ||
(this.superClass !== null &&
this.superClass.hasEffectsWhenCalledAtPath(path, callOptions, context))
);
if (callOptions.withNew) {
return path.length > 0 ||
(this.classConstructor !== null &&
this.classConstructor.hasEffectsWhenCalledAtPath(EMPTY_PATH, callOptions, context)) ||
(this.superClass !== null &&
this.superClass.hasEffectsWhenCalledAtPath(path, callOptions, context));
} else {
if (path.length !== 1 || this.deoptimizedStatic) return true;
const key = path[0];
const definition = typeof key === 'string' && this.getStaticPropertyMap()[key];
if (!definition) return true;
return definition.hasEffectsWhenCalledAtPath([], callOptions, context);
}
}

initialise() {
if (this.id !== null) {
this.id.declare('class', this);
}
for (const method of this.body.body) {
if (method instanceof MethodDefinition && method.kind === 'constructor') {
this.classConstructor = method;
return;
}
}
this.classConstructor = null;
}

mayModifyThisWhenCalledAtPath(
path: ObjectPath
) {
const key = path[0];
const definition = typeof key === 'string' && this.getStaticPropertyMap()[key];
if (!definition || this.deoptimizedStatic) return true;
return definition.mayModifyThisWhenCalledAtPath(path.slice(1));
}

private getStaticPropertyMap(): {[name: string]: ExpressionNode} {
if (this.staticPropertyMap) return this.staticPropertyMap;

const propertyMap = this.staticPropertyMap = Object.create(null);
const seen: {[name: string]: boolean} = Object.create(null);
for (const definition of this.body.body) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!definition.static) continue;
// If there are non-identifier-named statics, give up.
if (definition.computed || !(definition.key instanceof Identifier)) {
return this.staticPropertyMap = Object.create(null);
}
const key = definition.key.name;
// Not handling duplicate definitions.
if (seen[key]) {
delete propertyMap[key];
continue;
}
seen[key] = true;
if (definition instanceof MethodDefinition) {
if (definition.kind === "method") {
propertyMap[key] = definition.value;
}
} else if (definition.value) {
propertyMap[key] = definition.value;
}
}
return this.staticPropertyMap = propertyMap;
}
}

function mayHaveGetterSetterEffect(
body: ClassBody,
kind: 'get' | 'set', isStatic: boolean, name: string,
context: HasEffectsContext
) {
for (const definition of body.body) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this could benefit from a pre-built lookup table.

if (definition instanceof MethodDefinition && definition.static === isStatic && definition.kind === kind) {
if (definition.computed || !(definition.key instanceof Identifier)) {
return true;
}
if (definition.key.name === name &&
definition.value.hasEffectsWhenCalledAtPath([], {args: [], withNew: false}, context)) {
return true;
}
}
}
return false;
}
3 changes: 3 additions & 0 deletions test/form/samples/literals-from-class-statics/_config.js
@@ -0,0 +1,3 @@
module.exports = {
description: 'tracks literal values in class static fields'
};
13 changes: 13 additions & 0 deletions test/form/samples/literals-from-class-statics/_expected.js
@@ -0,0 +1,13 @@
log("t");
log("x");

class Undef {
static y;
}
if (Undef.y) log("y");

class Deopt {
static z = false;
}
unknown(Deopt);
if (Deopt.z) log("z");
21 changes: 21 additions & 0 deletions test/form/samples/literals-from-class-statics/main.js
@@ -0,0 +1,21 @@
class Static {
static t() { return true; }
static f() { return false; }
static x = 10;
}

if (Static.t()) log("t");
if (Static.f()) log("f");
if (!Static.t()) log("!t");
if (Static.x) log("x");

class Undef {
static y;
}
if (Undef.y) log("y");

class Deopt {
static z = false;
}
unknown(Deopt);
if (Deopt.z) log("z");
@@ -0,0 +1,3 @@
module.exports = {
description: 'treat getters and setters on classes as function calls'
};
35 changes: 35 additions & 0 deletions test/form/samples/side-effects-class-getters-setters/_expected.js
@@ -0,0 +1,35 @@
class RetainedByGetter {
get a() { log(); }
}
RetainedByGetter.prototype.a;

class RetainedBySetter {
set a(v) { log(); }
}
RetainedBySetter.prototype.a = 10;

class RetainedByStaticGetter {
static get a() { log(); }
}
RetainedByStaticGetter.a;

class RetainedByStaticSetter {
static set a(v) { log(); }
}
RetainedByStaticSetter.a = 10;

class RetainedSuper {
static get a() { log(); }
}
class RetainedSub extends RetainedSuper {}
RetainedSub.a;

class DeoptProto {}
unknown(DeoptProto.prototype);
DeoptProto.prototype.a;

class DeoptComputed {
static get a() {}
static get [unknown]() { log(); }
}
DeoptComputed.a;