Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8486015:
Adds support for indexing of JS files (Closed)
|
Created:
9 years, 1 month ago by mmendez Modified:
9 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionThis is not meant to be a general purpose indexer, but it is meant to be sufficient for conservative tree shaking of dartc generated JS code and the native code of the various libraries.
Committed: https://code.google.com/p/dart/source/detail?r=1656
Patch Set 1 #
Total comments: 16
Patch Set 2 : Updates based on zundel and codefu's feedback. #
Total comments: 2
Patch Set 3 : Added missing newline #
Total comments: 16
Patch Set 4 : Address fabiomfv's feedback #
Messages
Total messages: 8 (0 generated)
This is the first of several commits which will introduce tree shaking into the dartc compiler for the incremental and non-incremental modes. Here is an overview of where we are going: "Non-incremental Tree Shaking" The technical issue that blocked progress on this front was the ability to know which dart "native" methods the native JS library code calls. Without this information we must conservatively emit any method that is marked as native. So, I've created a very simple JS indexer/analyzer that allows me to conservatively answer the question. Once this infrastructure lands we will be able to tree shake and avoid code generation for large swaths of dart code as well as eliding RTT information when the program doesn't actually use it. This variant will be able to produce the smallest possible code. "Incremental Tree Shaking" Doing true tree shaking in the incremental compiler would require architectural changes. For now, we will leverage the JS indexer/analyzer by letting the incremental compile emit the code as it normally would but with an additional step to reduce the code size via tree shaking. If this proves too heavy handed for the IDE incremental compilation scenarios we can revisit the architectural changes. As one data point, the stock dartc can do a clean compile of total in 6.5s on a cold JVM and produces 10.7MB of JS code. With the pragmatic, conservative JS tree shaking, dartc can produce the 10.7MB, analyze it and tree shake it down to 3MB in 8.5s on a cold JVM -- and the application works. This is still a lot of code, but this moves us in the right direction and gives us some breathing room to contemplate the right next action.
Other than the one nit, LGTM http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:70: // A will be added and A.blah will be added I think this comment is copied in error.
LGTM, just nits http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... File compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java:62: public int getLength() { Most of the methods are self-explanatory and don't require javadoc, but I don't understand what the length of an element means. Looking at AstNode.getLength() didn't help. Does it mean the number of characters in the output this element takes up? http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... File compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:135: System.out.println(ctorCost + " characters worth of $Constructor and $Initializer methode declarations"); > 100 chars http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:162: ExpressionStatement expressionStatement = (ExpressionStatement) node; one or two comments about what this if stmt is doing would be much appreciated. http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:218: // have a whitelist of funny formatting http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:221: new JavascriptElement(null, false, enclosingTypeName, nameLocator.getName(), null); > 100 chars http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:48: new HashMap<String, List<JavascriptElement>>(); Maps.newHashMap() simplifies this. http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:223: } I don't see any tests setting properties on top level var statements. That is, of the form: var A = ... A.Hello = function() { } I doubt there are problems, but all the same, its valid JS and could be used in native js
Updated patch attached. http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... File compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java:62: public int getLength() { Yeah, the AstNode API is not precise, but yes it is the number of characters that the element consumes in the output JS. I added a javadoc comment to this method. On 2011/11/17 18:20:32, zundel wrote: > Most of the methods are self-explanatory and don't require javadoc, but I don't > understand what the length of an element means. Looking at AstNode.getLength() > didn't help. Does it mean the number of characters in the output this element > takes up? http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... File compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:135: System.out.println(ctorCost + " characters worth of $Constructor and $Initializer methode declarations"); On 2011/11/17 18:20:32, zundel wrote: > > 100 chars Done. http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:162: ExpressionStatement expressionStatement = (ExpressionStatement) node; Sorry about that -- it is some subtle code. I added javadoc to the method and added some comments to the body as well. On 2011/11/17 18:20:32, zundel wrote: > one or two comments about what this if stmt is doing would be much appreciated. http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:218: // have a whitelist of On 2011/11/17 18:20:32, zundel wrote: > funny formatting Done. http://codereview.chromium.org/8486015/diff/1/compiler/java/com/google/dart/c... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:221: new JavascriptElement(null, false, enclosingTypeName, nameLocator.getName(), null); On 2011/11/17 18:20:32, zundel wrote: > > 100 chars Done. http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:48: new HashMap<String, List<JavascriptElement>>(); On 2011/11/17 18:20:32, zundel wrote: > Maps.newHashMap() simplifies this. Done. http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:70: // A will be added and A.blah will be added Good catch. I updated the comment accordingly. On 2011/11/17 15:18:22, codefu wrote: > I think this comment is copied in error. http://codereview.chromium.org/8486015/diff/1/compiler/javatests/com/google/d... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:223: } Added a test for this case. On 2011/11/17 18:20:32, zundel wrote: > I don't see any tests setting properties on top level var statements. That is, > of the form: > > var A = ... > A.Hello = function() { } > > I doubt there are problems, but all the same, its valid JS and could be used in > native js
still LGTM w/ nit http://codereview.chromium.org/8486015/diff/5001/compiler/javatests/com/googl... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/5001/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:177: public void testTopLevelFunctions() { missing n/l
Updated patch http://codereview.chromium.org/8486015/diff/5001/compiler/javatests/com/googl... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/5001/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:177: public void testTopLevelFunctions() { On 2011/11/17 18:54:18, zundel wrote: > missing n/l Done.
LGTM! some nits. I would be nice if you could write in the CL description the size gains for empty, hello world and total. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java (right): http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java:96: * expression is found "Array.prototype.XXX = function(){}". why we need this again? should the example you mentioned in the comment be something else? http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java:103: public boolean isNative() { pls add a comment explaining what native means in this context (it has nothing to do with dart 'native' but with js 'built-in' types). http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java (right): http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:38: private final LinkedList<String> identifiers = new LinkedList<String>(); nit. List<String> on left side. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:88: public static void printGlobals(List<AstNode> globals) { nit. since this is debugging code, we could comment it out. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:97: public static void printNamesToElements(Map<String, List<JavascriptElement>> namesToElements) { nit. same here. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:176: if ("RunEntry".equals(targetName.getIdentifier())) { nit. it would be nice to put "RunEntry" is in a class const instead of hardcoded. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:192: subtypeFunction.setInherits(namesToElements.get(supertype.getIdentifier()).get(0)); I wonder if 'shouldWarnOnNoSuchType' option is on if there may be a case where super type name is missing and cause a npe.could you double check? http://codereview.chromium.org/8486015/diff/2003/compiler/javatests/com/googl... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/2003/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:235: assertEquals(3, namesToElements.size()); curiosity - why should it be 3?
I still have 2 more CLs to land to enable the "incremental" tree shaking. I'll update that CL with the final code size reductions for incremental tree shaking. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java (right): http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java:96: * expression is found "Array.prototype.XXX = function(){}". This comment belongs on line 103. We do need to know if a member was instantiated, used in a new expression and we assume that "JS" native classes are always instantiated. On 2011/11/17 20:41:27, fabiomfv wrote: > why we need this again? should the example you mentioned in the comment be > something else? http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/JavascriptElement.java:103: public boolean isNative() { On 2011/11/17 20:41:27, fabiomfv wrote: > pls add a comment explaining what native means in this context (it has nothing > to do with dart 'native' but with js 'built-in' types). Done. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java (right): http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:38: private final LinkedList<String> identifiers = new LinkedList<String>(); Loosened to Deque. On 2011/11/17 20:41:27, fabiomfv wrote: > nit. List<String> on left side. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:88: public static void printGlobals(List<AstNode> globals) { That is true. On the other hand since it is a static method there is no harm in leaving it in place since it does not introduce any warnings into the code. We should discuss offline so we can approach consistency. On 2011/11/17 20:41:27, fabiomfv wrote: > nit. since this is debugging code, we could comment it out. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:97: public static void printNamesToElements(Map<String, List<JavascriptElement>> namesToElements) { See above. On 2011/11/17 20:41:27, fabiomfv wrote: > nit. same here. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:176: if ("RunEntry".equals(targetName.getIdentifier())) { On 2011/11/17 20:41:27, fabiomfv wrote: > nit. it would be nice to put "RunEntry" is in a class const instead of > hardcoded. Done. http://codereview.chromium.org/8486015/diff/2003/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexer.java:192: subtypeFunction.setInherits(namesToElements.get(supertype.getIdentifier()).get(0)); We treat missing supertypes as errors now. So, I don't think this could happen. Even in the generate-code-in-the-presence-of-errors scenario. On 2011/11/17 20:41:27, fabiomfv wrote: > I wonder if 'shouldWarnOnNoSuchType' option is on if there may be a case where > super type name is missing and cause a npe.could you double check? http://codereview.chromium.org/8486015/diff/2003/compiler/javatests/com/googl... File compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java (right): http://codereview.chromium.org/8486015/diff/2003/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/backend/js/analysis/TopLevelElementIndexerTest.java:235: assertEquals(3, namesToElements.size()); We add entries into the map for Array, Array.prototype.foo and foo. Array is an implicit native object, and the remaining two entries deal with the static and virtual references to foo. On 2011/11/17 20:41:27, fabiomfv wrote: > curiosity - why should it be 3? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
