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

Disable pessimistic object deoptimization for calls when the called function doesn't ref this #4011

Merged
merged 6 commits into from Mar 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 2 additions & 1 deletion src/ast/nodes/CallExpression.ts
Expand Up @@ -65,7 +65,8 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
// ensure the returnExpression is set for the tree-shaking passes
this.getReturnExpression(SHARED_RECURSION_TRACKER);
// This deoptimizes "this" for non-namespace calls until we have a better solution
if (this.callee instanceof MemberExpression && !this.callee.variable) {
if (this.callee instanceof MemberExpression && !this.callee.variable &&
this.callee.mayModifyThisWhenCalledAtPath([])) {
this.callee.object.deoptimizePath(UNKNOWN_PATH);
}
for (const argument of this.arguments) {
Expand Down
6 changes: 6 additions & 0 deletions src/ast/nodes/Identifier.ts
Expand Up @@ -141,6 +141,12 @@ export default class Identifier extends NodeBase implements PatternNode {
this.variable!.includeCallArguments(context, args);
}

mayModifyThisWhenCalledAtPath(
path: ObjectPath
) {
return this.variable ? this.variable.mayModifyThisWhenCalledAtPath(path) : true
}

render(
code: MagicString,
_options: RenderOptions,
Expand Down
9 changes: 9 additions & 0 deletions src/ast/nodes/MemberExpression.ts
Expand Up @@ -233,6 +233,15 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
this.propertyKey = getResolvablePropertyKey(this);
}

mayModifyThisWhenCalledAtPath(
path: ObjectPath
) {
if (this.variable) {
return this.variable.mayModifyThisWhenCalledAtPath(path);
}
return this.object.mayModifyThisWhenCalledAtPath([this.propertyKey as ObjectPathKey].concat(path));
}

render(
code: MagicString,
options: RenderOptions,
Expand Down
10 changes: 10 additions & 0 deletions src/ast/nodes/ObjectExpression.ts
Expand Up @@ -256,6 +256,16 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
return false;
}

mayModifyThisWhenCalledAtPath(
path: ObjectPath
) {
if (!path.length || typeof path[0] !== "string") {
return true;
}
const property = this.getPropertyMap()[path[0]]?.exactMatchRead;
return property ? property.value.mayModifyThisWhenCalledAtPath(path.slice(1)) : true;
}

render(
code: MagicString,
options: RenderOptions,
Expand Down
7 changes: 7 additions & 0 deletions src/ast/nodes/ThisExpression.ts
Expand Up @@ -4,6 +4,7 @@ import ModuleScope from '../scopes/ModuleScope';
import { ObjectPath } from '../utils/PathTracker';
import ThisVariable from '../variables/ThisVariable';
import * as NodeType from './NodeType';
import FunctionNode from './shared/FunctionNode';
import { NodeBase } from './shared/Node';

export default class ThisExpression extends NodeBase {
Expand Down Expand Up @@ -38,6 +39,12 @@ export default class ThisExpression extends NodeBase {
this.start
);
}
for (let parent = this.parent; parent instanceof NodeBase; parent = parent.parent) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like this use of a for loop.

if (parent instanceof FunctionNode) {
parent.referencesThis = true;
break;
}
}
}

render(code: MagicString) {
Expand Down
3 changes: 3 additions & 0 deletions src/ast/nodes/shared/Expression.ts
Expand Up @@ -33,4 +33,7 @@ export interface ExpressionEntity extends WritableEntity {
): boolean;
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void;
includeCallArguments(context: InclusionContext, args: (ExpressionNode | SpreadElement)[]): void;
mayModifyThisWhenCalledAtPath(
path: ObjectPath
): boolean;
}
10 changes: 10 additions & 0 deletions src/ast/nodes/shared/FunctionNode.ts
Expand Up @@ -16,12 +16,16 @@ export default class FunctionNode extends NodeBase {
id!: IdentifierWithVariable | null;
params!: PatternNode[];
preventChildBlockScope!: true;
referencesThis!: boolean;
scope!: FunctionScope;

private isPrototypeDeoptimized = false;

createScope(parentScope: FunctionScope) {
this.scope = new FunctionScope(parentScope, this.context);
// Initialized here because child nodes will update it before
// `this.initialize` even runs.
this.referencesThis = false;
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Maybe move it rather into parseNode, which is where "initialisation before initialise" usually happens. I.e. parseNode is guaranteed to run before child nodes are even created.

}

deoptimizePath(path: ObjectPath) {
Expand Down Expand Up @@ -120,6 +124,12 @@ export default class FunctionNode extends NodeBase {
this.body.addImplicitReturnExpressionToScope();
}

mayModifyThisWhenCalledAtPath(
path: ObjectPath
) {
return path.length ? true : this.referencesThis
}

parseNode(esTreeNode: GenericEsTreeNode) {
this.body = new this.context.nodeConstructors.BlockStatement(
esTreeNode.body,
Expand Down
4 changes: 4 additions & 0 deletions src/ast/nodes/shared/MultiExpression.ts
Expand Up @@ -73,4 +73,8 @@ export class MultiExpression implements ExpressionEntity {
}

includeCallArguments(): void {}

mayModifyThisWhenCalledAtPath(path: ObjectPath) {
return this.expressions.some(e => e.mayModifyThisWhenCalledAtPath(path));
}
}
6 changes: 6 additions & 0 deletions src/ast/nodes/shared/Node.ts
Expand Up @@ -229,6 +229,12 @@ export class NodeBase implements ExpressionNode {
}
}

mayModifyThisWhenCalledAtPath(
_path: ObjectPath
) {
return true
}

parseNode(esTreeNode: GenericEsTreeNode) {
for (const key of Object.keys(esTreeNode)) {
// That way, we can override this function to add custom initialisation and then call super.parseNode
Expand Down