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

add custom closure compiler pass #2593

Merged
merged 2 commits into from
Apr 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
.DS_Store
.g4ignore
build
build-system/build/classes
c
dist
dist.3p
Expand All @@ -12,3 +11,5 @@ node_modules
npm-debug.log
.idea
.tm_properties
.settings
build-system/runner/TESTS-TestSuites.xml
4 changes: 3 additions & 1 deletion build-system/runner/.classpath
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
<?xml version="1.0" encoding="UTF-8"?>
<classpath>
<classpathentry including="**/*.java" kind="src" path="src"/>
<classpathentry including="**/*.java" kind="src" output="build/test-classes" path="test"/>
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/>
<classpathentry kind="lib" path="lib/compiler.jar" sourcepath="/Users/erwinm/dev/java/closure-compiler"/>
<classpathentry kind="lib" path="./../../third_party/closure-compiler/compiler.jar"/>
<classpathentry kind="lib" path="./../../third_party/closure-compiler/compiler-and-tests.jar"/>
<classpathentry kind="output" path="build/classes"/>
</classpath>
90 changes: 86 additions & 4 deletions build-system/runner/build.xml
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@
<!--define folder properties-->
<property name="dir.buildfile" value="."/>
<property name="src.dir" value="${basedir}/src"/>
<property name="gen.dir" value="${basedir}/gen"/>
<property name="test.dir" value="${basedir}/test"/>
<property name="build.dir" value="${basedir}/build"/>
<property name="lib.dir" value="${basedir}/lib"/>
<property name="dist.dir" value="${basedir}/dist"/>
<property name="runner-jarfile" value="${build.dir}/${ant.project.name}.jar"/>
<property name="compiler.dir" value="${basedir}/../../third_party/closure-compiler"/>

<property name="classes.dir" value="${build.dir}/classes"/>
<property name="testClasses.dir" value="${build.dir}/test"/>
Expand All @@ -33,7 +34,7 @@
<jar destfile="${runner-jarfile}">
<zipfileset src="${lib.dir}/jar-in-jar-loader.zip"/>
<fileset dir="${classes.dir}"/>
<fileset dir="${lib.dir}">
<fileset dir="${compiler.dir}">
<include name="compiler.jar"/>
</fileset>
<manifest>
Expand All @@ -43,11 +44,12 @@
<attribute name="Rsrc-Class-Path" value="./ compiler.jar"/>
</manifest>
</jar>
<mkdir dir="${dist.dir}"/>
<copy file="${build.dir}/runner.jar" todir="${dist.dir}"/>
</target>

<target name="compile">
<mkdir dir="${classes.dir}"/>
<copy file="${src.dir}/../../../third_party/closure-compiler/compiler.jar" todir="${lib.dir}"/>
<javac srcdir="${src.dir}"
destdir="${classes.dir}"
excludes=".git,.DS_Store"
Expand All @@ -56,20 +58,100 @@
<classpath refid="srcclasspath.path"/>
<compilerarg value="-Xlint:unchecked"/>
</javac>
<!-- This copy is needed for tests -->
<copy file="${compiler.dir}/Messages.properties"
todir="${classes.dir}/com/google/javascript/rhino/"/>
<!-- This copy is needed for tests -->
<copy file="${compiler.dir}/ParserConfig.properties"
todir="${classes.dir}/com/google/javascript/jscomp/parsing"/>
</target>

<target name="clean" description="delete generated files">
<delete dir="${build.dir}"/>
</target>

<target name="test" depends="compile-tests">
<mkdir dir="build/testoutput"/>
<junit printsummary="on" fork="${test.fork}"
forkmode="once" showoutput="true"
dir="${basedir}"
failureproperty="junit.failure">
<classpath refid="allclasspath.path" />
<classpath>
<pathelement location="${build.dir}/test"/>
</classpath>
<batchtest todir="build/testoutput">
<formatter type="brief" usefile="false"/>
<formatter type="xml"/>
<fileset dir="${build.dir}/test">
<include name="**/${test.class}.class"/>
</fileset>
</batchtest>
</junit>
<junitreport>
<fileset dir="build/testoutput" includes="*.xml"/>
<report todir="build/testoutput"/>
</junitreport>
<fail if="junit.failure" message="Unit tests failed. See build/testoutput/index.html"/>
</target>

<target name="compile-tests" depends="compile">
<mkdir dir="${testClasses.dir}"/>
<javac srcdir="${src.dir}"
destdir="${testClasses.dir}"
excludes=".git,**/debugger/**,.DS_Store"
debug="on"
deprecation="on"
includeantruntime="false"
encoding="utf-8">
<classpath refid="allclasspath.path"/>
<compilerarg value="-Xlint:unchecked"/>
</javac>
<javac srcdir="${test.dir}"
destdir="${testClasses.dir}"
excludes=".git,.DS_Store"
debug="on"
deprecation="on"
includeantruntime="false"
encoding="utf-8">
<classpath refid="allclasspath.path"/>
</javac>
</target>

<target name="one-test" depends="compile-tests">
<mkdir dir="build/testoutput"/>
<junit printsummary="on" fork="${test.fork}"
forkmode="once" showoutput="true"
dir="${basedir}"
failureproperty="junit.failure">
<classpath refid="allclasspath.path"/>
<classpath>
<pathelement location="${build.dir}/test"/>
</classpath>
<test todir="build/testoutput" name="${test.class}" methods="${test.method}">
<formatter type="brief" usefile="false"/>
<formatter type="xml"/>
</test>
</junit>
<junitreport>
<fileset dir="build/testoutput" includes="*.xml"/>
<report todir="build/testoutput"/>
</junitreport>
<fail if="junit.failure"
message="Unit tests failed. See build/testoutput/index.html"/>
</target>

<path id="srcclasspath.path">
<pathelement location="${classes.dir}"/>
<fileset dir="${lib.dir}">
<fileset dir="${compiler.dir}">
<include name="compiler.jar"/>
</fileset>
</path>

<path id="allclasspath.path">
<pathelement location="${classes.dir}"/>
<fileset dir="${compiler.dir}">
<include name="compiler-and-tests.jar"/>
</fileset>
</path>
</project>
Binary file not shown.
13 changes: 13 additions & 0 deletions build-system/runner/runner.iml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
<module type="JAVA_MODULE" version="4">
<component name="NewModuleRootManager" inherit-compiler-output="true">
<exclude-output />
<content url="file://$MODULE_DIR$">
<sourceFolder url="file://$MODULE_DIR$/src" isTestSource="false" />
<sourceFolder url="file://$MODULE_DIR$/test" isTestSource="true" />
</content>
<orderEntry type="inheritedJdk" />
<orderEntry type="sourceFolder" forTests="false" />
</component>
</module>

19 changes: 17 additions & 2 deletions build-system/runner/src/org/ampproject/AmpCommandLineRunner.java
Original file line number Diff line number Diff line change
@@ -1,21 +1,36 @@
package org.ampproject;


import com.google.common.collect.ImmutableSet;
import com.google.javascript.jscomp.CommandLineRunner;
import com.google.javascript.jscomp.CompilerOptions;
import com.google.javascript.jscomp.CustomPassExecutionTime;


/**
* Adds a way to add custom options and custom compiler passes.
* Adds a custom pass for Tree shaking `dev.fine` and `dev.assert` calls.
*/
public class AmpCommandLineRunner extends CommandLineRunner {

/**
* List of string suffixes to eliminate from the AST.
*/
ImmutableSet<String> suffixTypes = ImmutableSet.of(
"dev.fine");

protected AmpCommandLineRunner(String[] args) {
super(args);
}

@Override protected CompilerOptions createOptions() {
CompilerOptions options = super.createOptions();
AmpPass ampPass = new AmpPass(getCompiler(), suffixTypes);
options.addCustomPass(CustomPassExecutionTime.BEFORE_OPTIMIZATIONS, ampPass);
return options;
}

public static void main(String[] args) {
AmpCommandLineRunner runner = new AmpCommandLineRunner(args);

if (runner.shouldRunCompiler()) {
runner.run();
}
Expand Down
132 changes: 132 additions & 0 deletions build-system/runner/src/org/ampproject/AmpPass.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package org.ampproject;

import java.util.Set;

import com.google.javascript.jscomp.AbstractCompiler;
import com.google.javascript.jscomp.HotSwapCompilerPass;
import com.google.javascript.jscomp.NodeTraversal;
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.rhino.Node;

/**
* Does a `stripTypeSuffix` which currently can't be done through
* the normal `strip` mechanisms provided by closure compiler.
* Some of the known mechanisms we tried before writing our own compiler pass
* are setStripTypes, setStripTypePrefixes, setStripNameSuffixes, setStripNamePrefixes.
Copy link
Member

Choose a reason for hiding this comment

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

Great comment.

* The normal mechanisms found in closure compiler can't strip the expressions we want because
* they are either prefix based and/or operate on the es6 translated code which would mean they
* operate on a qualifier string name that looks like
* "module$__$__$__$extensions$amp_test$0_1$log.dev.fine".
*
* Other custom pass examples found inside closure compiler src:
* https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/PolymerPass.java
* https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/AngularPass.java
*/
class AmpPass extends AbstractPostOrderCallback implements HotSwapCompilerPass {

final AbstractCompiler compiler;
private final Set<String> stripTypeSuffixes;

public AmpPass(AbstractCompiler compiler, Set<String> stripTypeSuffixes) {
this.compiler = compiler;
this.stripTypeSuffixes = stripTypeSuffixes;
}

@Override public void process(Node externs, Node root) {
hotSwapScript(root, null);
}

@Override public void hotSwapScript(Node scriptRoot, Node originalRoot) {
NodeTraversal.traverseEs6(compiler, scriptRoot, this);
}

@Override public void visit(NodeTraversal t, Node n, Node parent) {
if (isDevAssertCall(n)) {
maybeEliminateCallExceptFirstParam(n, parent);
} else if (n.isExprResult()) {
maybeEliminateExpressionBySuffixName(n, parent);
}
}

private boolean isDevAssertCall(Node n) {
if (n.isCall()) {
Node expression = n.getFirstChild();
if (expression == null) {
return false;
}

String name = expression.getQualifiedName();
if (name == null) {
return false;
}

if (name.endsWith("dev.assert")) {
return true;
}
}
return false;
}

/**
* Checks if expression is a GETPROP() (method invocation) and the property
* name ends with one of the items in stripTypeSuffixes.
*/
private void maybeEliminateExpressionBySuffixName(Node n, Node parent) {
// n = EXPRESSION_RESULT > CALL > GETPROP
Node call = n.getFirstChild();
if (call == null) {
return;
}
Node expression = call.getFirstChild();
if (expression == null) {
return;
}

if (qualifiedNameEndsWithStripType(expression)) {
if (parent.isExprResult()) {
Copy link
Member

Choose a reason for hiding this comment

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

You currently just kill the full statement, right?

I wonder if, for asserts, you should just alway copy the first argument here. It would be DCEed if it is side effect free.

Copy link
Member Author

Choose a reason for hiding this comment

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

right. should we just go ahead and change that for all typeSuffixes we look for or specifically just for assert?

Copy link
Member

Choose a reason for hiding this comment

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

Just for assert. We should just completely kill logging.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Node grandparent = parent.getParent();
grandparent.removeChild(parent);
} else {
parent.removeChild(n);
}
compiler.reportCodeChange();
}
}

private void maybeEliminateCallExceptFirstParam(Node n, Node p) {
Node functionName = n.getFirstChild();
if (functionName == null) {
return;
}
Node firstArg = functionName.getNext();
if (firstArg == null) {
return;
}
firstArg.detachFromParent();
p.replaceChild(n, firstArg);
compiler.reportCodeChange();
}

/**
* Checks the nodes qualified name if it ends with one of the items in
* stripTypeSuffixes
*/
boolean qualifiedNameEndsWithStripType(Node n) {
String name = n.getQualifiedName();
return qualifiedNameEndsWithStripType(name);
}

/**
* Checks if the string ends with one of the items in stripTypeSuffixes
*/
boolean qualifiedNameEndsWithStripType(String name) {
if (name != null) {
for (String suffix : stripTypeSuffixes) {
if (name.endsWith(suffix)) {
return true;
}
}
}
return false;
}
}
Loading