Skip to content

Commit

Permalink
Tighten the dead property elimination deoptimzation
Browse files Browse the repository at this point in the history 
The previous implemention was a little too restrictive for most environments.
We now only trigger a deop if any of the following is true:

  a. A Object.defineProperty(...) where an aliased property name is being
  assigned to an aliased descriptor or the descriptor contains a getter/setter
  def.
  b. A Object.defineProperties where the last param is not an object literal.

To ensure safety, a property being defined with an aliased descriptor now just
blacklists that property name.

Finally, a new diagnostic error is introduced to report when there is a
pattern that triggers a critical deoptimization.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124891113
  • Loading branch information
@kevinoconnor7 @blickly
kevinoconnor7 authored and blickly committed Jun 15, 2016
1 parent d07eed8 commit a593a49
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 25 deletions.
62 changes: 44 additions & 18 deletions src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java
View file
Expand Up @@ -314,14 +314,14 @@ private void visitBlock(Node blockNode) {
}

private static boolean isConditionalExpression(Node n) {
switch (n.getType()) {
switch (n.getKind()) {
case AND:
case OR:
case HOOK:
return true;
default:
return false;
}

return false;
}

private void visitAssignmentLhs(Node lhs) {
Expand Down Expand Up @@ -363,7 +363,7 @@ private void visitAssignmentLhs(Node lhs) {
}

private boolean visitNode(Node n, Node parent) {
switch (n.getType()) {
switch (n.getKind()) {
case GETPROP:
// Handle potential getters/setters.
if (n.isGetProp()
Expand Down Expand Up @@ -480,22 +480,22 @@ public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent)
@Override
public void visit(NodeTraversal t, Node n, Node parent) {
if (NodeUtil.isObjectDefinePropertyDefinition(n)) {
// We must assume any property in the compilation can be a getter/setter if any of the
// params are potentially aliasing the real values.
if (!NodeUtil.isPrototypeProperty(n.getSecondChild())
|| !n.getChildAtIndex(2).isString()
|| !n.getChildAtIndex(3).isObjectLit()) {
unknownGetterSetterPresent = true;
return;
}
} else if (NodeUtil.isObjectDefinePropertiesDefinition(n)) {
// If the first param is not a prototype reference or the second param is not an object
// literal then we must assume any property in the compilation can be a getter/setter.
if (!NodeUtil.isPrototypeProperty(n.getSecondChild())
|| !n.getChildAtIndex(2).isObjectLit()) {
// We must assume any property in the compilation can be a getter/setter if the property
// name and what is being assigned to are aliased.
if (!n.getChildAtIndex(2).isString() && !n.getLastChild().isObjectLit()) {
unknownGetterSetterPresent = true;
return;
} else if (!n.getLastChild().isObjectLit()) {
// If know the property name but not what it's being assigned to then we need to blackist
// the property name.
propNames.add(n.getChildAtIndex(2).getString());
}
return;
} else if (NodeUtil.isObjectDefinePropertiesDefinition(n)
&& !n.getChildAtIndex(2).isObjectLit()) {
// If the second param is not an object literal then we must assume any property in the
// compilation can be a getter/setter.
unknownGetterSetterPresent = true;
return;
}

// Keep track of any potential getters/setters.
Expand All @@ -509,13 +509,39 @@ public void visit(NodeTraversal t, Node n, Node parent) {
Node propNode = grandparent.getChildAtIndex(2);
if (propNode.isString()) {
propNames.add(propNode.getString());
} else {
// Putting a getter/setter on an aliased property means any property can be a getter or
// setter.
unknownGetterSetterPresent = true;
}
} else if (grandparent.isStringKey()
&& NodeUtil.isObjectDefinePropertiesDefinition(grandparent.getParent().getParent())) {
// Handle Object.defineProperties(obj, {propName: { ... }}).
propNames.add(grandparent.getString());
}
} else if (isAliasedPropertySet(n)) {
// If we know this property is being injected but don't know if there's a getter/setter
// then the property still must be blacklisted.
propNames.add(n.getString());
}
}

/**
* Determines if the given keyNode contains an aliased property set. In particular this is only
* true if the grandparent is an {@code Object.defineProperties} call.
*
* <p>Ex. {@code Object.defineProperties(Foo.prototype, {bar: someObj}}.
*/
private static boolean isAliasedPropertySet(Node keyNode) {
if (keyNode == null || !keyNode.isStringKey() || keyNode.getParent() == null) {
return false;
}

Node objectLit = keyNode.getParent();
return objectLit.getParent() != null
&& NodeUtil.isObjectDefinePropertiesDefinition(objectLit.getParent())
&& objectLit.getParent().getLastChild() == objectLit
&& !keyNode.getFirstChild().isObjectLit();
}
}
}
99 changes: 92 additions & 7 deletions test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java
View file
Expand Up @@ -849,7 +849,7 @@ public void testEs5Setter() {
);
}

public void testObjectDefineProperty_aliasedVars() {
public void testObjectDefineProperty_aliasedParams() {
testSame(
LINE_JOINER.join(
"function addGetter(obj, propName) {",
Expand Down Expand Up @@ -891,7 +891,9 @@ public void testObjectDefineProperty_aliasedVars() {
" x.bar = 10;",
" x.bar = 20;",
"}"));
}

public void testObjectDefineProperty_aliasedObject() {
testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
Expand All @@ -910,7 +912,9 @@ public void testObjectDefineProperty_aliasedVars() {
" x.bar = 10;",
" x.bar = 20;",
"}"));
}

public void testObjectDefineProperty_aliasedPropName() {
testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
Expand All @@ -930,6 +934,44 @@ public void testObjectDefineProperty_aliasedVars() {
" x.bar = 20;",
"}"));

test(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"var x = 'bar';",
"Object.defineProperty(Foo.prototype, x, {",
" value: 10",
"});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" foo.bar = 10;",
" foo.enabled = false;",
"}",
"function z() {",
" var x = {};",
" x.bar = 10;",
" x.bar = 20;",
"}"),
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"var x = 'bar';",
"Object.defineProperty(Foo.prototype, x, {",
" value: 10",
"});",
"function f() {",
" var foo = new Foo()",
" true;",
" foo.bar = 10;",
" foo.enabled = false;",
"}",
"function z() {",
" var x = {};",
" 10;",
" x.bar = 20;",
"}"));
}

public void testObjectDefineProperty_aliasedPropSet() {
testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
Expand All @@ -950,17 +992,14 @@ public void testObjectDefineProperty_aliasedVars() {
"}"));
}

public void testObjectDefineProperties_aliasedVars() {
public void testObjectDefineProperties_aliasedPropertyMap() {
testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"var properties = {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}};",
"var x = Foo.prototype;",
"Object.defineProperties(x, {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}});",
"Object.defineProperties(Foo.prototype, properties);",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
Expand All @@ -974,17 +1013,63 @@ public void testObjectDefineProperties_aliasedVars() {
"}"));

testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"var properties = {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"};",
"Object.defineProperties(Foo.prototype, {bar: properties});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" foo.bar = 10;",
" foo.enabled = false;",
"}",
"function z() {",
" var x = {};",
" x.bar = 10;",
" x.bar = 20;",
"}"));
}

public void testObjectDefineProperties_aliasedObject() {
test(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"var properties = {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}};",
"Object.defineProperties(Foo.prototype, properties);",
"var x = Foo.prototype;",
"Object.defineProperties(x, {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" foo.bar = 10;",
" foo.enabled = false;",
" foo.enabled = true;",
"}",
"function z() {",
" var x = {};",
" x.bar = 10;",
" x.bar = 20;",
"}"),
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"var properties = {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}};",
"var x = Foo.prototype;",
"Object.defineProperties(x, {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" foo.bar = 10;",
" false;",
" foo.enabled = true;",
"}",
"function z() {",
" var x = {};",
Expand Down

0 comments on commit a593a49

Please sign in to comment.