Skip to content

Commit

Permalink
Automated g4 rollback of changelist 173304853.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

breakages

*** Original change description ***

Improve the compile time cost for Optimize Calls from #1 to #17 (roughly 25% of its previous time).

Instead of using the DefinitionUseSiteFinder, as simpler data structure is introduced: OptimizeCalls.ReferenceMap.  The ReferenceMap provides a simple list of nodes for every global name and property. This avoids building objects and structures, and identifiers that are unneeded, and avoided redundant analysis.

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173326949
  • Loading branch information
@concavelenz @blickly
concavelenz authored and blickly committed Oct 25, 2017
1 parent 13d7a57 commit c1ea905
Show file tree
Hide file tree
Showing 20 changed files with 923 additions and 1,634 deletions.
9 changes: 9 additions & 0 deletions src/com/google/javascript/jscomp/AbstractCompiler.java
View file
Expand Up @@ -281,6 +281,15 @@ static enum MostRecentTypechecker {
*/
abstract <T extends TypeIRegistry> T getGlobalTypeInfo();

/**
* Used by three passes that run in sequence (optimize-returns,
* optimize-parameters, remove-unused-variables), to avoid having them
* recompute it independently.
*/
abstract DefinitionUseSiteFinder getDefinitionFinder();

abstract void setDefinitionFinder(DefinitionUseSiteFinder defFinder);

abstract void setExternExports(String externExports);

/**
Expand Down
9 changes: 6 additions & 3 deletions src/com/google/javascript/jscomp/AstValidator.java
View file
Expand Up @@ -475,8 +475,12 @@ private void validateTemplateLit(Node n) {
if (!n.hasChildren()) {
return;
}
for (Node child = n.getFirstChild(); child != null; child = child.getNext()) {
if (child.isString()) {
int i = 0;
for (Node child = n.getFirstChild(); child != null; child = child.getNext(), i++) {
// If the first child is not a STRING, this is a tagged template.
if (i == 0 && !child.isString()) {
validateExpression(child);
} else if (child.isString()) {
validateString(child);
} else {
validateTemplateLitSub(child);
Expand Down Expand Up @@ -600,7 +604,6 @@ private void validateClassMember(Node n, boolean isAmbient) {
case MEMBER_FUNCTION_DEF:
case GETTER_DEF:
case SETTER_DEF:
validateObjectLiteralKeyName(n);
validateChildCount(n);
Node function = n.getFirstChild();
if (isAmbient) {
Expand Down
6 changes: 0 additions & 6 deletions src/com/google/javascript/jscomp/ChangeVerifier.java
View file
Expand Up @@ -211,12 +211,6 @@ private static boolean isEquivalentToExcludingFunctions(
}
}

if (thisNode.getParent() != null && thisNode.getParent().isParamList()) {
if (thisNode.isUnusedParameter() != thatNode.isUnusedParameter()) {
return false;
}
}

Node thisChild = thisNode.getFirstChild();
Node thatChild = thatNode.getFirstChild();
while (thisChild != null && thatChild != null) {
Expand Down
13 changes: 13 additions & 0 deletions src/com/google/javascript/jscomp/Compiler.java
View file
Expand Up @@ -227,6 +227,9 @@ public SourceFile loadSource(String filename) {

public PerformanceTracker tracker;

// Used by optimize-returns, optimize-parameters and remove-unused-variables
private DefinitionUseSiteFinder defFinder = null;

// Types that have been forward declared
private Set<String> forwardDeclaredTypes = new HashSet<>();

Expand Down Expand Up @@ -1685,6 +1688,16 @@ GlobalTypeInfo getGlobalTypeInfo() {
return this.globalTypeInfo;
}

@Override
DefinitionUseSiteFinder getDefinitionFinder() {
return this.defFinder;
}

@Override
void setDefinitionFinder(DefinitionUseSiteFinder defFinder) {
this.defFinder = defFinder;
}

public void maybeSetTracker() {
if (options.getTracerMode().isOn()) {
PrintStream tracerOutput =
Expand Down
33 changes: 22 additions & 11 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -970,6 +970,10 @@ private List<PassFactory> getMainOptimizationLoop() {
passes.add(inlineProperties);
}

boolean runOptimizeCalls = options.optimizeCalls
|| options.optimizeParameters
|| options.optimizeReturns;

if (options.removeUnusedVars || options.removeUnusedLocalVars) {
if (options.deadAssignmentElimination) {
passes.add(deadAssignmentsElimination);
Expand All @@ -982,14 +986,19 @@ private List<PassFactory> getMainOptimizationLoop() {
passes.add(deadPropertyAssignmentElimination);
}
}
if (!runOptimizeCalls) {
passes.add(getRemoveUnusedVars(PassNames.REMOVE_UNUSED_VARS, false));
}
}

if (options.optimizeCalls || options.optimizeParameters || options.optimizeReturns) {
if (runOptimizeCalls) {
passes.add(optimizeCalls);
}

if (options.removeUnusedVars || options.removeUnusedLocalVars) {
passes.add(getRemoveUnusedVars());
// RemoveUnusedVars cleans up after optimizeCalls, so we run it here.
// It has a special name because otherwise PhaseOptimizer would change its
// position in the optimization loop.
if (options.optimizeCalls) {
passes.add(getRemoveUnusedVars("removeUnusedVars_afterOptimizeCalls", true));
}
}

if (options.j2clPassMode.shouldAddJ2clPasses()) {
Expand Down Expand Up @@ -2740,17 +2749,18 @@ protected CompilerPass create(AbstractCompiler compiler) {
}
};

private PassFactory getRemoveUnusedVars() {
return getRemoveUnusedVars(false /* isOneTimePass */);
private PassFactory getRemoveUnusedVars(String name, final boolean modifyCallSites) {
return getRemoveUnusedVars(name, modifyCallSites, false /* isOneTimePass */);
}

private PassFactory lastRemoveUnusedVars() {
return getRemoveUnusedVars(true /* isOneTimePass */);
return getRemoveUnusedVars(PassNames.REMOVE_UNUSED_VARS, false, true /* isOneTimePass */);
}

private PassFactory getRemoveUnusedVars(boolean isOneTimePass) {
private PassFactory getRemoveUnusedVars(
String name, final boolean modifyCallSites, boolean isOneTimePass) {
/** Removes variables that are never used. */
return new PassFactory(PassNames.REMOVE_UNUSED_VARS, isOneTimePass) {
return new PassFactory(name, isOneTimePass) {
@Override
protected CompilerPass create(AbstractCompiler compiler) {
boolean removeOnlyLocals = options.removeUnusedLocalVars && !options.removeUnusedVars;
Expand All @@ -2759,7 +2769,8 @@ protected CompilerPass create(AbstractCompiler compiler) {
return new RemoveUnusedVars(
compiler,
!removeOnlyLocals,
preserveAnonymousFunctionNames);
preserveAnonymousFunctionNames,
modifyCallSites);
}

@Override
Expand Down
4 changes: 3 additions & 1 deletion src/com/google/javascript/jscomp/DevirtualizePrototypeMethods.java
View file
Expand Up @@ -58,7 +58,8 @@
* </pre>
*
*/
class DevirtualizePrototypeMethods implements CompilerPass {
class DevirtualizePrototypeMethods
implements OptimizeCalls.CallGraphCompilerPass, CompilerPass {
private final AbstractCompiler compiler;

DevirtualizePrototypeMethods(AbstractCompiler compiler) {
Expand All @@ -72,6 +73,7 @@ public void process(Node externs, Node root) {
process(externs, root, defFinder);
}

@Override
public void process(
Node externs, Node root, DefinitionUseSiteFinder definitions) {
for (DefinitionSite defSite : definitions.getDefinitionSites()) {
Expand Down
60 changes: 16 additions & 44 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -3061,13 +3061,6 @@ static boolean isFunctionExpression(Node n) {
return n.isFunction() && (!isNamedFunction(n) || !isDeclarationParent(n.getParent()));
}

/**
* @return Whether the node is both a function expression and the function is named.
*/
static boolean isNamedFunctionExpression(Node n) {
return NodeUtil.isFunctionExpression(n) && !n.getFirstChild().getString().isEmpty();
}

/**
* see {@link #isFunctionExpression}
*
Expand Down Expand Up @@ -3131,39 +3124,13 @@ static boolean isEmptyFunctionExpression(Node node) {
* Determines if a function takes a variable number of arguments by
* looking for references to the "arguments" var_args object.
*/
@Deprecated
static boolean isVarArgsFunction(Node fn) {
return doesFunctionReferenceOwnArgumentsObject(fn);
}

/**
* @return Whether a function has a reference to its own "arguments" object.
*/
static boolean doesFunctionReferenceOwnArgumentsObject(Node fn) {
checkArgument(fn.isFunction());
if (fn.isArrowFunction()) {
return false;
}
return referencesArgumentsHelper(fn.getLastChild());
}

/** @return Whether the predicate is true for the node or any of its descendants. */
private static boolean referencesArgumentsHelper(Node node) {
if (node.isName() && node.getString().equals("arguments")) {
return true;
}

if (NodeUtil.isVanillaFunction(node)) {
return false;
}

for (Node c = node.getFirstChild(); c != null; c = c.getNext()) {
if (referencesArgumentsHelper(c)) {
return true;
}
}

return false;
static boolean isVarArgsFunction(Node function) {
// TODO(johnlenz): rename this function
checkArgument(function.isFunction());
return !function.isArrowFunction() && isNameReferenced(
function.getLastChild(),
"arguments",
MATCH_NOT_THIS_BINDING);
}

/**
Expand Down Expand Up @@ -4915,9 +4882,10 @@ static String getBestLValueName(@Nullable Node lValue) {
}

/**
* @return true iff the result of the expression is consumed.
* @return false iff the result of the expression is not consumed.
*/
static boolean isExpressionResultUsed(Node expr) {
// TODO(johnlenz): consider sharing some code with trySimpleUnusedResult.
Node parent = expr.getParent();
switch (parent.getToken()) {
case BLOCK:
Expand Down Expand Up @@ -4946,9 +4914,13 @@ static boolean isExpressionResultUsed(Node expr) {

return (expr == parent.getFirstChild()) ? false : isExpressionResultUsed(parent);
case FOR:
// Only an expression whose result is in the condition part of the
// expression is used.
return (parent.getSecondChild() == expr);
case FOR_IN:
if (!parent.isForIn()) {
// Only an expression whose result is in the condition part of the
// expression is used.
return (parent.getSecondChild() == expr);
}
break;
default:
break;
}
Expand Down

0 comments on commit c1ea905

Please sign in to comment.