add custom closure compiler pass #2593

erwinmombay · 2016-03-15T18:12:24Z

We add a custom closure compiler pass called AmpPass which allows us to do some optimizations like assert and logging elimination.

dvoytenko · 2016-03-15T18:14:13Z

third_party/closure-compiler/src/AmpCommandLineRunner.java

+
+  @Override
+  protected CompilerOptions createOptions() {
+    CompilerOptions options = super.createOptions();


How do we test this? Should we first try it w/o LOG_STRIP_TYPES and see if the result is equivalent?

@dvoytenko absolutely. trying to get a local green test first before since local is super flakey right now =/. i also need to add its integration with the gulp build pipeline

added JUnit tests.

erwinmombay · 2016-03-16T23:44:27Z

third_party/closure-compiler/runner/src/AmpCommandLineRunner.java

+  @Override
+  protected CompilerOptions createOptions() {
+    CompilerOptions options = super.createOptions();
+    options.setStripTypes(Sets.newHashSet(LOG_STRIP_SUFFIX));


We have 2 mechanisms to do DCE, setStripTypes/setStripTypePrefixes (option A) and setStripNameSuffixes/setStripNamePrefixes (option B). option A is more safer as we can declare an expression like dev.fine while option B seems to only allow fine or assert (no access operation, just bare method/property name).

the issue is that the strip operation seems to occur after the SIMPLE_PLUS_OPTIMIZATION has ran (only reason i can think of why it isn't working), meaning if you did setStripTypes('dev.fine') (option A), it wouldn't work, as the actual js code it operates on would be something like a.fine (after var renaming), this is also made worse by the output code for modules having the module name as the prefix of invocations, which means the output of dev.log is actually a.dev.log (where a is the log module). Now we could use option B, but option B is extremely unsafe since fine or assert are very generic and any other module/class could declare one, and to get around this we would need the log module to declare method names like devfine and devassert (even then it feels extremely unsafe).

One other option we can do is assign the log instances to the global AMP object since that is never renamed and an expression like AMP.dev.fine('hello world') would be stripped out if we do setStripTypes('AMP.dev.fine'). But this would also mean log has to be the very first module loaded since it would be needed by the other modules that uses logging.

/cc @cramforce @dvoytenko

This is crazy inconvenient. Ideally we'd find a way to do stripping before renaming/obfuscation. No idea why they thought it'd be a good idea. A stopgap option might be to disable dev/user obfuscation? Obviously that's a bit annoying too, but since we considering to doing them as globals, it doesn't really matter. And most of uses of these APIs are dev.fine and dev.assert - so it might be ok anyway.

yeah, i might just invoke addCustomPass which i think allows me to walk the AST. will try it out

so ive been researching it some more and its not that its running after optimization, its that it runs against the es6 -> es5 transpiled code (with module prefix symbols) meaning its walking through something that looks like the screenshot below (where its already been prefixed with the namespace):

erwinmombay · 2016-03-17T11:36:08Z

third_party/closure-compiler/runner/src/AmpPass.java

+
+  boolean qualifiedNameEndsWithStripType(String name) {
+    if (name != null) {
+      return name.endsWith("log.dev.fine");


@dvoytenko @cramforce this is what i came up with which is based on closure compilers' stripTypePrefixes code, this code is more or less a stripTypeSuffixes (which isnt implemented)

cramforce · 2016-03-17T16:53:00Z

third_party/closure-compiler/runner/src/AmpCommandLineRunner.java

+      runner.run();
+    }
+    if (runner.hasErrors()) {
+      System.exit(-1);


Not 1? Do they get printed out?

this is an exact copy of the main code in the super class, and in my testing the errors/warning do get printed out.

This library is on GitHub? Can we sprinkle links/references to the "standard" code here?

yep, will do.

cramforce · 2016-03-17T16:56:00Z

Looks great overall.

In a follow up we should take flags out of the JS config and instead set them in Java.

dvoytenko · 2016-03-17T17:20:47Z

third_party/closure-compiler/runner/.classpath

+<classpath>
+	<classpathentry kind="src" path="src"/>
+	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/>
+	<classpathentry kind="lib" path="/Users/erwinm/dev/amphtml/third_party/closure-compiler/compiler.jar"/>


Is this file actually needed? If so - can we make this a local file path?

not needed, will clean up the project artifacts

erwinmombay · 2016-04-19T08:18:40Z

@cramforce one slightly big issue is the addition of the 2 semi large jar files. runner.jar (5.9mb) and compiler-and-tests.jar (10.4mb, which is a redundant build of compiler.jar + tests, this is so i can inherit from base test classes for testing)

cramforce · 2016-04-19T14:28:35Z

build-system/runner/amp-test/0.1/test.js

+  return a;
+}
+
+function sideEffectAssert() {


This is not actually a side effect. Ideally this would compile to
return 'wahhh'

If that is not what you currently do it may be fine for now, but we should be correct on terminology.

For side effects I'd expects something like

dev.assert(document.body.appendChild(…))

to compile to

document.body.appendChild(…)

haha sorry, this file is really just for testing. i'll remove this

erwinmombay · 2016-04-22T17:49:43Z

build-system/runner/test/org/ampproject/AmpPassTest.java

+  }
+
+  public void testDevAssertPreserveFirstArg() throws Exception {
+    test(


/cc @cramforce

erwinmombay · 2016-04-22T17:50:16Z

@cramforce PTAL

cramforce · 2016-04-22T18:08:34Z

LGTM

erwinmombay · 2016-04-22T18:42:41Z

test/size.txt

@@ -1,38 +1,38 @@
      max   |         min   |       gzip   |   file
      ---   |         ---   |        ---   |   ---
-612.52 kB   |   178.05 kB   |   49.15 kB   |   v0.js / amp.js


/cc @dvoytenko for log elimination new sizes

dvoytenko reviewed Mar 15, 2016
View reviewed changes

erwinmombay force-pushed the runner branch 6 times, most recently from 05a2417 to 5c5f91d Compare March 16, 2016 08:25

erwinmombay changed the title ~~[WIP] add custom closure compiler runner~~ add custom closure compiler runner Mar 16, 2016

erwinmombay force-pushed the runner branch 2 times, most recently from 5161682 to 0f71025 Compare March 16, 2016 23:37

erwinmombay changed the title ~~add custom closure compiler runner~~ [WIP] add custom closure compiler runner Mar 16, 2016

erwinmombay force-pushed the runner branch 4 times, most recently from 4555e6b to e5f0dff Compare March 16, 2016 23:40

erwinmombay reviewed Mar 16, 2016
View reviewed changes

erwinmombay force-pushed the runner branch 2 times, most recently from 7408bba to 758b160 Compare March 17, 2016 11:33

erwinmombay reviewed Mar 17, 2016
View reviewed changes

erwinmombay force-pushed the runner branch from 9e06c04 to ba74e44 Compare March 17, 2016 11:58

cramforce reviewed Mar 17, 2016
View reviewed changes

dvoytenko reviewed Mar 17, 2016
View reviewed changes

erwinmombay force-pushed the runner branch from ba74e44 to 6357d98 Compare April 4, 2016 17:15

erwinmombay force-pushed the runner branch 4 times, most recently from 635fe3c to e2f7f3d Compare April 19, 2016 07:22

erwinmombay force-pushed the runner branch 5 times, most recently from 87f9cdf to 0172e01 Compare April 19, 2016 08:26

cramforce reviewed Apr 19, 2016
View reviewed changes

erwinmombay force-pushed the runner branch from 0172e01 to 81e0e87 Compare April 19, 2016 18:36

changes to third_party/closure-compiler

8a278d5

erwinmombay force-pushed the runner branch 4 times, most recently from 4b18605 to 92f9f14 Compare April 22, 2016 17:49

erwinmombay reviewed Apr 22, 2016
View reviewed changes

erwinmombay force-pushed the runner branch 2 times, most recently from dd4cca2 to bf4fb11 Compare April 22, 2016 17:54

cramforce added LGTM and removed NEEDS REVIEW labels Apr 22, 2016

add custom closure compiler pass

48a8425

erwinmombay force-pushed the runner branch from bf4fb11 to 48a8425 Compare April 22, 2016 18:42

erwinmombay reviewed Apr 22, 2016
View reviewed changes

erwinmombay merged commit 656c3ec into ampproject:master Apr 22, 2016

erwinmombay changed the title ~~add custom closure compiler runner~~ add custom closure compiler pass Apr 22, 2016

erwinmombay mentioned this pull request May 8, 2016

Strip logging from production binary #2497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add custom closure compiler pass #2593

add custom closure compiler pass #2593

erwinmombay commented Mar 15, 2016 •

edited

Loading

dvoytenko Mar 15, 2016

erwinmombay Mar 15, 2016

erwinmombay Apr 19, 2016

erwinmombay Mar 16, 2016

dvoytenko Mar 17, 2016

erwinmombay Mar 17, 2016

erwinmombay Mar 17, 2016

erwinmombay Mar 17, 2016

cramforce Mar 17, 2016

erwinmombay Mar 17, 2016

dvoytenko Mar 17, 2016

erwinmombay Mar 17, 2016

cramforce commented Mar 17, 2016

dvoytenko Mar 17, 2016

erwinmombay Mar 17, 2016

erwinmombay commented Apr 19, 2016 •

edited

Loading

cramforce Apr 19, 2016

cramforce Apr 19, 2016

erwinmombay Apr 19, 2016

erwinmombay Apr 22, 2016

cramforce Apr 22, 2016

erwinmombay commented Apr 22, 2016

cramforce commented Apr 22, 2016

erwinmombay Apr 22, 2016

cramforce Apr 22, 2016

add custom closure compiler pass #2593

add custom closure compiler pass #2593

Conversation

erwinmombay commented Mar 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Apr 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Apr 22, 2016

cramforce commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Mar 15, 2016 •

edited

Loading

erwinmombay commented Apr 19, 2016 •

edited

Loading