Skip to content

Commit

Permalink
Expand OptimizeCalls to conditionally consider externs.
Browse files Browse the repository at this point in the history 
A builder for `OptimzeCalls` instances is defined for setting this configuration. All current instantiations are updated to use this builder.

This additional functionality is not currently used. It is prework for switching `PureFunctionIdentifier` to be based off `OptimizeCalls`. Pure function identification must also inspect externs.

Very basic tests for the reference collecting behaviour of `OptimizeCalls` are also added covering both the extern and non-extern case.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=240583621
  • Loading branch information
@nreid260 @blickly
nreid260 authored and blickly committed Mar 27, 2019
1 parent 647aad3 commit 029603b
Show file tree
Hide file tree
Showing 8 changed files with 737 additions and 490 deletions.
22 changes: 13 additions & 9 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -2504,9 +2504,11 @@ protected FeatureSet featureSet() {
new PassFactory(PassNames.DEVIRTUALIZE_PROTOTYPE_METHODS, true) { new PassFactory(PassNames.DEVIRTUALIZE_PROTOTYPE_METHODS, true) {
@Override @Override
protected CompilerPass create(AbstractCompiler compiler) { protected CompilerPass create(AbstractCompiler compiler) {
OptimizeCalls passes = new OptimizeCalls(compiler); return OptimizeCalls.builder()
passes.addPass(new DevirtualizePrototypeMethods(compiler)); .setCompiler(compiler)
return passes; .setConsiderExterns(false)
.addPass(new DevirtualizePrototypeMethods(compiler))
.build();
} }


@Override @Override
Expand All @@ -2523,12 +2525,14 @@ protected FeatureSet featureSet() {
new PassFactory(PassNames.OPTIMIZE_CALLS, false) { new PassFactory(PassNames.OPTIMIZE_CALLS, false) {
@Override @Override
protected CompilerPass create(AbstractCompiler compiler) { protected CompilerPass create(AbstractCompiler compiler) {
OptimizeCalls passes = new OptimizeCalls(compiler); return OptimizeCalls.builder()
// Remove unused return values. .setCompiler(compiler)
passes.addPass(new OptimizeReturns(compiler)); .setConsiderExterns(false)
// Remove all parameters that are constants or unused. // Remove unused return values.
passes.addPass(new OptimizeParameters(compiler)); .addPass(new OptimizeReturns(compiler))
return passes; // Remove all parameters that are constants or unused.
.addPass(new OptimizeParameters(compiler))
.build();
} }


@Override @Override
Expand Down
112 changes: 79 additions & 33 deletions src/com/google/javascript/jscomp/OptimizeCalls.java
View file
Expand Up @@ -17,11 +17,13 @@
package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Preconditions.checkState;


import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.javascript.jscomp.AbstractCompiler.LifeCycleStage;
import com.google.javascript.jscomp.NodeTraversal.ScopedCallback; import com.google.javascript.jscomp.NodeTraversal.ScopedCallback;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import java.util.ArrayList; import java.util.ArrayList;
Expand All @@ -45,30 +47,82 @@
* @author johnlenz@google.com (John Lenz) * @author johnlenz@google.com (John Lenz)
*/ */
class OptimizeCalls implements CompilerPass { class OptimizeCalls implements CompilerPass {
private final List<CallGraphCompilerPass> passes = new ArrayList<>();
private final AbstractCompiler compiler; private final AbstractCompiler compiler;
private final ImmutableList<CallGraphCompilerPass> passes;
private final boolean considerExterns;


OptimizeCalls(AbstractCompiler compiler) { private OptimizeCalls(
AbstractCompiler compiler,
ImmutableList<CallGraphCompilerPass> passes,
boolean considerExterns) {
this.compiler = compiler; this.compiler = compiler;
this.passes = passes;
this.considerExterns = considerExterns;
}

static Builder builder() {
return new Builder();
} }


interface CallGraphCompilerPass { interface CallGraphCompilerPass {
void process(Node externs, Node root, ReferenceMap references); void process(Node externs, Node root, ReferenceMap references);
} }


OptimizeCalls addPass(CallGraphCompilerPass pass) { static final class Builder {
passes.add(pass); private AbstractCompiler compiler;
return this; private ImmutableList.Builder<CallGraphCompilerPass> passes = ImmutableList.builder();
@Nullable private Boolean considerExterns; // Nullable to force users to specify a value.

public Builder setCompiler(AbstractCompiler compiler) {
this.compiler = compiler;
return this;
}

/**
* Sets whether or not to include references to extern names and properties in the {@link
* ReferenceMap} being generated.
*
* <p>If considered, references to externs in both extern code <em>and</em> executable code will
* be collected. Otherwise, neither will be.
*
* <p>This setting allows extern references to be effectively invisible to passes that should
* not mutate them.
*/
public Builder setConsiderExterns(boolean b) {
this.considerExterns = b;
return this;
}

public Builder addPass(CallGraphCompilerPass pass) {
this.passes.add(pass);
return this;
}

public OptimizeCalls build() {
checkNotNull(considerExterns);

return new OptimizeCalls(compiler, passes.build(), considerExterns);
}

private Builder() {}
} }


@Override @Override
public void process(Node externs, Node root) { public void process(Node externs, Node root) {
if (!passes.isEmpty()) { // Only global names are collected, which is insufficient if names have not been normalized.
ReferenceMap refMap = buildPropAndGlobalNameReferenceMap( checkState(compiler.getLifeCycleStage() == LifeCycleStage.NORMALIZED);
compiler, externs, root);
for (CallGraphCompilerPass pass : passes) { if (passes.isEmpty()) {
pass.process(externs, root, refMap); return;
} }

final ReferenceMap references = new ReferenceMap();
NodeTraversal.traverseRoots(
compiler, new ReferenceMapBuildingCallback(references), externs, root);

for (CallGraphCompilerPass pass : passes) {
pass.process(externs, root, references);
} }
} }


Expand Down Expand Up @@ -308,8 +362,7 @@ private static Set<String> safeSet(@Nullable Set<String> set) {
return (set != null) ? ImmutableSet.copyOf(set) : ImmutableSet.of(); return (set != null) ? ImmutableSet.copyOf(set) : ImmutableSet.of();
} }


static class ReferenceMapBuildingCallback implements ScopedCallback { private final class ReferenceMapBuildingCallback implements ScopedCallback {
AbstractCompiler compiler;
final Set<String> externProps; final Set<String> externProps;
final ReferenceMap references; final ReferenceMap references;
private Scope globalScope; private Scope globalScope;
Expand All @@ -318,8 +371,7 @@ static class ReferenceMapBuildingCallback implements ScopedCallback {
* @param compiler * @param compiler
* @param references * @param references
*/ */
public ReferenceMapBuildingCallback(AbstractCompiler compiler, ReferenceMap references) { public ReferenceMapBuildingCallback(ReferenceMap references) {
this.compiler = compiler;
this.externProps = safeSet(compiler.getExternProperties()); this.externProps = safeSet(compiler.getExternProperties());
this.references = references; this.references = references;
} }
Expand Down Expand Up @@ -360,27 +412,29 @@ public void visit(NodeTraversal t, Node n, Node unused) {


private void maybeAddNameReference(Node n) { private void maybeAddNameReference(Node n) {
String name = n.getString(); String name = n.getString();
if (isGlobalNonExternNameReference(name)) { Var var = globalScope.getSlot(name);
if (var != null && (considerExterns || !var.isExtern())) {
// As every name declaration is unique due to normalizations, it is only necessary to build
// the global scope and ask it if it knows about a name as it can never be shadowed.
references.addNameReference(name, n); references.addNameReference(name, n);
} }
} }


private void maybeAddPropReference(String name, Node n) { private void maybeAddPropReference(String name, Node n) {
if (!externProps.contains(name)) { if (considerExterns || !externProps.contains(name)) {
references.addPropReference(name, n); references.addPropReference(name, n);
} }
} }


// As every name declaration is unique due to normalizations, it is only necessary to build
// the global scope and ask it if it knows about a name as it can never be shadowed.
private boolean isGlobalNonExternNameReference(String name) {
Var v = globalScope.getSlot(name);
return v != null && !v.isExtern();
}

@Override @Override
public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
return !n.isScript() || !t.getInput().isExtern(); if (n.isFromExterns()) {
// Even when considering externs, we only care about top-level identifiers. Dummy function
// parameters, for example, shouldn't be considered references.
return considerExterns && t.inGlobalScope();
} else {
return true;
}
} }


@Override @Override
Expand All @@ -396,14 +450,6 @@ public void exitScope(NodeTraversal t) {
} }
} }


static ReferenceMap buildPropAndGlobalNameReferenceMap(
AbstractCompiler compiler, Node externs, Node root) {
final ReferenceMap references = new ReferenceMap();
NodeTraversal.traverseRoots(compiler, new ReferenceMapBuildingCallback(
compiler, references), externs, root);
return references;
}

/** /**
* @return Whether the provide name may be a candidate for * @return Whether the provide name may be a candidate for
* call optimizations. * call optimizations.
Expand Down
10 changes: 7 additions & 3 deletions src/com/google/javascript/jscomp/OptimizeParameters.java
View file
Expand Up @@ -56,9 +56,13 @@ class OptimizeParameters implements CompilerPass, OptimizeCalls.CallGraphCompile
@VisibleForTesting @VisibleForTesting
public void process(Node externs, Node root) { public void process(Node externs, Node root) {
checkState(compiler.getLifeCycleStage() == LifeCycleStage.NORMALIZED); checkState(compiler.getLifeCycleStage() == LifeCycleStage.NORMALIZED);
ReferenceMap refMap = OptimizeCalls.buildPropAndGlobalNameReferenceMap(
compiler, externs, root); OptimizeCalls.builder()
process(externs, root, refMap); .setCompiler(compiler)
.setConsiderExterns(false)
.addPass(this)
.build()
.process(externs, root);
} }


@Override @Override
Expand Down
9 changes: 6 additions & 3 deletions src/com/google/javascript/jscomp/OptimizeReturns.java
View file
Expand Up @@ -47,9 +47,12 @@ class OptimizeReturns implements OptimizeCalls.CallGraphCompilerPass, CompilerPa
@Override @Override
@VisibleForTesting @VisibleForTesting
public void process(Node externs, Node root) { public void process(Node externs, Node root) {
ReferenceMap refMap = OptimizeCalls.buildPropAndGlobalNameReferenceMap( OptimizeCalls.builder()
compiler, externs, root); .setCompiler(compiler)
process(externs, root, refMap); .setConsiderExterns(false)
.addPass(this)
.build()
.process(externs, root);
} }


@Override @Override
Expand Down
24 changes: 13 additions & 11 deletions test/com/google/javascript/jscomp/DevirtualizePrototypeMethodsTest.java
View file
Expand Up @@ -51,6 +51,7 @@ protected int getNumRepetitions() {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
super.setUp(); super.setUp();
enableNormalize(); // Required for `OptimizeCalls`.
disableTypeCheck(); disableTypeCheck();
} }


Expand Down Expand Up @@ -460,9 +461,9 @@ public void testRewrite_ifMultipleIdenticalDefinitions_withThis() {


@Test @Test
public void testRewrite_ifMultipleIdenticalDefinitions_withLocalNames() { public void testRewrite_ifMultipleIdenticalDefinitions_withLocalNames() {
// TODO(nickreid): This is actually dangerous, however because `Normalize` is run before // This case is included for completeness. `Normalization` is a prerequisite for this pass so
// devirtualization, this shouldn't ever happen. All local names will be unique. Maybe back off // the naming conflict is resolved before devirtualization even begins. The change in names
// in this case too. // invalidates devirtualization by making the definition subtrees unequal.
test( test(
lines( lines(
// Note how `f` refers to different objects in the function bodies, even though the // Note how `f` refers to different objects in the function bodies, even though the
Expand All @@ -475,14 +476,13 @@ public void testRewrite_ifMultipleIdenticalDefinitions_withLocalNames() {
"", "",
"x.getFoo();"), "x.getFoo();"),
lines( lines(
"function A() {}; ", "function A() {};",
"var JSCompiler_StaticMethods_getFoo =", "A.prototype.getFoo = function f() { return f.prop; }; ",
" function f(JSCompiler_StaticMethods_getFoo$self) { return f.prop; };",
"", "",
"function B() {};", "function B() {};",
"B.prototype.getFoo = function f() { return f.prop; };", // Dead definition. "B.prototype.getFoo = function f$jscomp$1() { return f$jscomp$1.prop; }; ",
"", "",
"JSCompiler_StaticMethods_getFoo(x);")); "x.getFoo();"));
} }


@Test @Test
Expand Down Expand Up @@ -1192,8 +1192,10 @@ public void testNoRewrite_definitionModule_afterUseModule() {


@Override @Override
protected CompilerPass getProcessor(Compiler compiler) { protected CompilerPass getProcessor(Compiler compiler) {
OptimizeCalls pass = new OptimizeCalls(compiler); return OptimizeCalls.builder()
pass.addPass(new DevirtualizePrototypeMethods(compiler)); .setCompiler(compiler)
return pass; .setConsiderExterns(false)
.addPass(new DevirtualizePrototypeMethods(compiler))
.build();
} }
} }

0 comments on commit 029603b

Please sign in to comment.