Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
90713003:
Dart2js option to dump info about compilation (Closed)
|
Created:
7 years ago by sigurdm Modified:
7 years ago CC:
reviews_dartlang.org, sethladd Visibility:
Public. |
DescriptionOption to dump information about compilation
This is a first draft of the feature.
The flag '--build-info' activates this feature.
The dump will go into out.js.info.html.
It contains information about used elements (libraries, functions,
fields, typedefs and classes) of the program:
- Inferred types
- generated code
- members
- size
R=johnniwinther@google.com
Committed: https://code.google.com/p/dart/source/detail?r=31060
Patch Set 1 #Patch Set 2 : #
Total comments: 60
Patch Set 3 : #
Total comments: 54
Patch Set 4 : #
Total comments: 22
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 38
Patch Set 7 : #
Total comments: 16
Patch Set 8 : #
Total comments: 4
Messages
Total messages: 21 (0 generated)
DBC: https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/compiler.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:117: sortedLibraries.sort( You could also write it as follows: (we usually format closures like this when given as arguments to reduce indentation) sortedLibraries.sort((LibraryElement l1, LibraryElement l2) { if (l1.isPlatformLibrary != l2.isPlatformLibrary) { return l1.isPlatformLibrary ? 1 : -1; } else { return l1.getLibraryName().compareTo(l2.getLibraryName()); } }); https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:127: List libraryInfos = []; 'List' -> 'List<Map>', maybe even add the key and value type. Also, add the argument type to '[]', for example '<Map>[]', otherwise the object will be a list of dynamic. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:128: for (LibraryElement library in sortedLibraries) { How about: libraryInfos.addAll(sortedLibraries .map((library) => library.collectInferredTypes(compiler)) .where((info) => info != null)); https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:135: "compilation duration" : compiler.totalCompileTime.elapsedTicks, Remove the space between the map key and ':'. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:32: buffer.write('<div class="kind">${description['name']}</div>' You could define helpers for these, like: String span({String cls}, String content) { return '<span${cls == null ? '' : ' class="$cls"'}>$content</span>'; } https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/elements.dart:262: * Collect a JSON Object describing inferred information on this element and 'JSON Object' -> Map. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1133: if (thisType == null) { Merge this int l. 1136 using '?:' ?
DBC:
Please use Dart, not jQuery. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:5: part of dart2js; This should be its own library. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:67: 'ajax/libs/jquery/2.0.3/jquery.min.js"></script>' WTF?
https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dart2js.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2js.dart:584: Given this option dart2js generates a out.info.html file with information "Given this option dart2js ..." is redundant.
Don't sweat it: it is standard practice to get a lot of style feedback when new on a project. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/compiler.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:86: final CodeSizeCounter codeSizeCounter = new CodeSizeCounter(); It doesn't seem appropriate for the backend to expose a codeSizeCounter. This appears to be a concern of dump_info.dart. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:88: CodeBuffer emitCodeFor(Element element); Please document this method. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:115: Map collectDumpInfo() { This seems like an odd fit for this functionality. Is there any way you can move this into dump_info.dart. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:1181: backend.dumpInfo(); This should be its own task. Then we can see how long it takes. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dart2jslib.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2jslib.dart:9: import 'dart:convert'; I'm pretty sure it is not appropriate to include this library in dart2js. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2jslib.dart:39: import 'dart2js.dart' as commandline; You cannot include this library. The dart2js command line is implemented in terms of the compiler API. The compiler API's implementation is this library. It does not include dart2js.dart, in particular because dart2js imports dart:io making dart2js unable to self-host. Hopefully some of our tests should catch this. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:149: return null; // TODO (sigurdm) Emit the code if available Why isn't this the default implementation? https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:151: Extra line. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:16: final Map<Element, int> _generatedSize = new Map<Element, int>(); Why is this field private? https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:18: int generatedSize(Element element) => I find this method rather peculiar. Sounds like a property, but it really is an action that I'm not sure what is supposed to do. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:18: int generatedSize(Element element) => Please use "return" when it doesn't fit on one line. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:21: countCode(Element element, int added) { Add void. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:62: buffer.writeln("<html>"); I feel this code could benefit tremendously from using a multiline string or even an external HTML file. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:27: CodeBuffer; This import doesn't belong here. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1004: Map collectInferredTypes(Compiler compiler) { This really doesn't seem to be an essential part of the element model. There are no inferred types in the element model, so elements cannot collect inferred types. I think it is time to add an element visitor an implement this using visitors. From an object oriented modeling perspective, there are some problematic things below, such as calling back to the backend to collect emitted code. Such things are not a concern of elements. However, if this code is implemented as a visitor, the visitor is concerned with collecting information from various sources, and thus it is a concern of the visitor to access diverse sources such as element model and backend data. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1019: "type" : this.canonicalUri.toString(), Generally, try to create classes instead of simple data types. Long term, that really improves maintainability. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1047: Extra lines. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1838: assert(invariant(this, allSupertypesAndSelf != null, This invariant is unnecessary. You get a null pointer when it fails. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:171: CodeBuffer emitCodeFor(Element element) { This method is unfortunately named as emitting normally is something that the emitter takes care of. I'd simply call this codeOf(element). Also, try to follow this class layout: Fields, Constructors, Accessors, Methods, Sometimes we intermix accessors and fields, but it really hurts readability in my opinion. Generally, when you add a new method to a class group it with related stuff (non I suspect in this case) or add it to the end. https://codereview.chromium.org/90713003/diff/20001/tests/compiler/dart2js/du... File tests/compiler/dart2js/dump_info_test.dart (right): https://codereview.chromium.org/90713003/diff/20001/tests/compiler/dart2js/du... tests/compiler/dart2js/dump_info_test.dart:51: ; ?
https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/compiler.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:86: final CodeSizeCounter codeSizeCounter = new CodeSizeCounter(); On 2013/11/27 19:28:53, ahe wrote: > It doesn't seem appropriate for the backend to expose a codeSizeCounter. This > appears to be a concern of dump_info.dart. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:88: CodeBuffer emitCodeFor(Element element); On 2013/11/27 19:28:53, ahe wrote: > Please document this method. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:115: Map collectDumpInfo() { On 2013/11/27 19:28:53, ahe wrote: > This seems like an odd fit for this functionality. Is there any way you can move > this into dump_info.dart. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:117: sortedLibraries.sort( On 2013/11/27 14:19:23, karlklose wrote: > You could also write it as follows: (we usually format closures like this when > given as arguments to reduce indentation) > > sortedLibraries.sort((LibraryElement l1, LibraryElement l2) { > if (l1.isPlatformLibrary != l2.isPlatformLibrary) { > return l1.isPlatformLibrary ? 1 : -1; > } else { > return l1.getLibraryName().compareTo(l2.getLibraryName()); > } > }); Much better https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:127: List libraryInfos = []; On 2013/11/27 14:19:23, karlklose wrote: > 'List' -> 'List<Map>', maybe even add the key and value type. Also, add the > argument type to '[]', for example '<Map>[]', otherwise the object will be a > list of dynamic. Problem is, that the value type is not fixed. But ok. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:128: for (LibraryElement library in sortedLibraries) { On 2013/11/27 14:19:23, karlklose wrote: > How about: > > libraryInfos.addAll(sortedLibraries > .map((library) => library.collectInferredTypes(compiler)) > .where((info) => info != null)); Nice https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:135: "compilation duration" : compiler.totalCompileTime.elapsedTicks, On 2013/11/27 14:19:23, karlklose wrote: > Remove the space between the map key and ':'. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:1181: backend.dumpInfo(); On 2013/11/27 19:28:53, ahe wrote: > This should be its own task. Then we can see how long it takes. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dart2js.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2js.dart:584: Given this option dart2js generates a out.info.html file with information On 2013/11/27 18:01:31, ahe wrote: > "Given this option dart2js ..." is redundant. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2js.dart:584: Given this option dart2js generates a out.info.html file with information On 2013/11/27 18:01:31, ahe wrote: > "Given this option dart2js ..." is redundant. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dart2jslib.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2jslib.dart:9: import 'dart:convert'; On 2013/11/27 19:28:53, ahe wrote: > I'm pretty sure it is not appropriate to include this library in dart2js. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart2jslib.dart:39: import 'dart2js.dart' as commandline; On 2013/11/27 19:28:53, ahe wrote: > You cannot include this library. > > The dart2js command line is implemented in terms of the compiler API. > > The compiler API's implementation is this library. It does not include > dart2js.dart, in particular because dart2js imports dart:io making dart2js > unable to self-host. Hopefully some of our tests should catch this. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:149: return null; // TODO (sigurdm) Emit the code if available On 2013/11/27 19:28:53, ahe wrote: > Why isn't this the default implementation? Good question https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:151: On 2013/11/27 19:28:53, ahe wrote: > Extra line. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:5: part of dart2js; On 2013/11/27 17:59:18, ahe wrote: > This should be its own library. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:16: final Map<Element, int> _generatedSize = new Map<Element, int>(); On 2013/11/27 19:28:53, ahe wrote: > Why is this field private? Because I miss a map with a default answer. But I can let the users use the member directly. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:18: int generatedSize(Element element) => On 2013/11/27 19:28:53, ahe wrote: > Please use "return" when it doesn't fit on one line. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:21: countCode(Element element, int added) { On 2013/11/27 19:28:53, ahe wrote: > Add void. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:32: buffer.write('<div class="kind">${description['name']}</div>' On 2013/11/27 14:19:23, karlklose wrote: > You could define helpers for these, like: > String span({String cls}, String content) { > return '<span${cls == null ? '' : ' class="$cls"'}>$content</span>'; > } Nice idea, done https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:62: buffer.writeln("<html>"); On 2013/11/27 19:28:53, ahe wrote: > I feel this code could benefit tremendously from using a multiline string or > even an external HTML file. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:67: 'ajax/libs/jquery/2.0.3/jquery.min.js"></script>' On 2013/11/27 17:59:18, ahe wrote: > WTF? I removed the folding view - we need a way to specify inline dart code... https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/elements.dart:262: * Collect a JSON Object describing inferred information on this element and On 2013/11/27 14:19:23, karlklose wrote: > 'JSON Object' -> Map. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:27: CodeBuffer; On 2013/11/27 19:28:53, ahe wrote: > This import doesn't belong here. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1004: Map collectInferredTypes(Compiler compiler) { On 2013/11/27 19:28:53, ahe wrote: > This really doesn't seem to be an essential part of the element model. > > There are no inferred types in the element model, so elements cannot collect > inferred types. > > I think it is time to add an element visitor an implement this using visitors. > > From an object oriented modeling perspective, there are some problematic things > below, such as calling back to the backend to collect emitted code. Such things > are not a concern of elements. However, if this code is implemented as a > visitor, the visitor is concerned with collecting information from various > sources, and thus it is a concern of the visitor to access diverse sources such > as element model and backend data. I agree I have made a visitor https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1019: "type" : this.canonicalUri.toString(), On 2013/11/27 19:28:53, ahe wrote: > Generally, try to create classes instead of simple data types. Long term, that > really improves maintainability. Yes - the map'o'maps was more for drafting purposes Done https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1047: On 2013/11/27 19:28:53, ahe wrote: > Extra lines. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1133: if (thisType == null) { On 2013/11/27 14:19:23, karlklose wrote: > Merge this int l. 1136 using '?:' ? Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1838: assert(invariant(this, allSupertypesAndSelf != null, On 2013/11/27 19:28:53, ahe wrote: > This invariant is unnecessary. You get a null pointer when it fails. Done. https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/20001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:171: CodeBuffer emitCodeFor(Element element) { On 2013/11/27 19:28:53, ahe wrote: > This method is unfortunately named as emitting normally is something that the > emitter takes care of. > > I'd simply call this codeOf(element). > > Also, try to follow this class layout: > > Fields, > Constructors, > Accessors, > Methods, > > Sometimes we intermix accessors and fields, but it really hurts readability in > my opinion. ok > > Generally, when you add a new method to a class group it with related stuff (non > I suspect in this case) or add it to the end. done https://codereview.chromium.org/90713003/diff/20001/tests/compiler/dart2js/du... File tests/compiler/dart2js/dump_info_test.dart (right): https://codereview.chromium.org/90713003/diff/20001/tests/compiler/dart2js/du... tests/compiler/dart2js/dump_info_test.dart:51: ; On 2013/11/27 19:28:53, ahe wrote: > ? Done.
https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:39: String name; Can some of these fields be final? https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:49: ElementInfo({String this.name: "", No need to add the types for the 'this.name' style parameters. The static type of such a parameter is always fetched from the corresponding field. Also, are all these parameters really optional or are you mostly interested in giving them a good name at the call site? https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:59: class ProgramInfo { Use final fields and no types for 'this.name' style constructor parameters (repeated in a few more places in this file). https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:83: "does not support dumping of types", 4 space indent. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:89: int size = compiler.dumpInfoTask.codeSizeCounter Doesn't this fit on one line? https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:99: String nameString = element.getLibraryName() == "" ? I suggest breaking before ? and formatting this along these lines: String name = (<boolean condition> ? <e0> : <e1>; https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:105: type: element.canonicalUri.toString(), 4 space indent. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:124: String modifiersString = element.modifiers.toString() == "" ? Consider the alternative ?: formatting with respect to line breaks (repeated in this file). https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:127: kind: "inferred type", 4 space indent. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:132: kind: "field", 4 space indent. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:155: contents.sort((ElementInfo m1, ElementInfo m2) { Why are these called m1 and m2? e1 and e2 seems more appropriate. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:159: kind: "class", 4 space indent (lots of occurrences). https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:187: type: compiler.typesTask Maybe the type parameter to the ElementInfo constructor should really be a TypeMask? It feels weird that you call toString everywhere. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:230: ..add(buffer.toString()) 4 space indent (match up to the next line). https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:246: List<ElementInfo> libraryInfos = new List<ElementInfo>.from(sortedLibraries Maybe compute the Iterable outside to make that code more readable? https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:251: compilationDuration: compiler.totalCompileTime.elapsedTicks, Weird indentation. Should be 4 space relative to the 'r' in return. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:262: return '<$element${cls == null ? '' : ' class="$cls"'}>' I'd compute the (cls == null ? '' : ' class ...') thing in a local variable to make this more readable. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:336: // TODO (sigurdm) this script should be written in dart Usually we format these TODO comment like this: // TODO(sigurdm): Foo bar quux. with a colon and no space between TODO and ( and a terminating dot. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:339: function toggler(element) { return function(e) { Newline before return and more indentation. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:340: element.hidden = ! element.hidden; No space after !. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:341: console.log(element); Do you really want to log this to the console? Looks like debugging code. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:345: for(var i = 0; i < containers.length; i++) { for( -> for ( https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:350: container.nextElementSibling.hidden=true; Spaces around =. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/visitor.dart (right): https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/visitor.dart:21: abstract class ElementVisitor<R> { I thought we agreed on not visiting the ModelX classes but only the more abstract interface types? What made you change your mind? I'd try to get rid of all the visitFooBarX methods in this visitor. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1950: if (!generatedCode.containsKey(functionElement)) return null; Use ?:. https://codereview.chromium.org/90713003/diff/40001/tests/compiler/dart2js/du... File tests/compiler/dart2js/dump_info_test.dart (right): https://codereview.chromium.org/90713003/diff/40001/tests/compiler/dart2js/du... tests/compiler/dart2js/dump_info_test.dart:22: () {} (); // TODO (sigurdm) empty closure, hack to avoid inlining See if you can use @NoInline() annotation and get rid of the TODO. https://codereview.chromium.org/90713003/diff/40001/tests/compiler/dart2js/du... tests/compiler/dart2js/dump_info_test.dart:37: Expect.stringEquals(mainlib.name, "main"); Expect.stringEquals expects the expected result as the first parameter, so you should swap the two arguments for all these calls.
https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:39: String name; On 2013/11/29 12:03:13, kasperl wrote: > Can some of these fields be final? all https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:49: ElementInfo({String this.name: "", On 2013/11/29 12:03:13, kasperl wrote: > No need to add the types for the 'this.name' style parameters. The static type > of such a parameter is always fetched from the corresponding field. Great! > > Also, are all these parameters really optional or are you mostly interested in > giving them a good name at the call site? A little of each https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:59: class ProgramInfo { On 2013/11/29 12:03:13, kasperl wrote: > Use final fields and no types for 'this.name' style constructor parameters > (repeated in a few more places in this file). Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:83: "does not support dumping of types", On 2013/11/29 12:03:13, kasperl wrote: > 4 space indent. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:89: int size = compiler.dumpInfoTask.codeSizeCounter On 2013/11/29 12:03:13, kasperl wrote: > Doesn't this fit on one line? No https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:99: String nameString = element.getLibraryName() == "" ? On 2013/11/29 12:03:13, kasperl wrote: > I suggest breaking before ? and formatting this along these lines: > > String name = (<boolean condition> > ? <e0> > : <e1>; Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:105: type: element.canonicalUri.toString(), On 2013/11/29 12:03:13, kasperl wrote: > 4 space indent. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:124: String modifiersString = element.modifiers.toString() == "" ? On 2013/11/29 12:03:13, kasperl wrote: > Consider the alternative ?: formatting with respect to line breaks (repeated in > this file). Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:127: kind: "inferred type", On 2013/11/29 12:03:13, kasperl wrote: > 4 space indent. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:132: kind: "field", On 2013/11/29 12:03:13, kasperl wrote: > 4 space indent. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:155: contents.sort((ElementInfo m1, ElementInfo m2) { On 2013/11/29 12:03:13, kasperl wrote: > Why are these called m1 and m2? e1 and e2 seems more appropriate. Looong story... fixed https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:159: kind: "class", On 2013/11/29 12:03:13, kasperl wrote: > 4 space indent (lots of occurrences). https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:187: type: compiler.typesTask On 2013/11/29 12:03:13, kasperl wrote: > Maybe the type parameter to the ElementInfo constructor should really be a > TypeMask? It feels weird that you call toString everywhere. Hmm - maybe - right now the "type" is some explanation of the element (superclasses for classes, types for fields, uri for libraries). I have renamed it to "details" maybe that is more appropriate. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:230: ..add(buffer.toString()) On 2013/11/29 12:03:13, kasperl wrote: > 4 space indent (match up to the next line). Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:246: List<ElementInfo> libraryInfos = new List<ElementInfo>.from(sortedLibraries On 2013/11/29 12:03:13, kasperl wrote: > Maybe compute the Iterable outside to make that code more readable? Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:251: compilationDuration: compiler.totalCompileTime.elapsedTicks, On 2013/11/29 12:03:13, kasperl wrote: > Weird indentation. Should be 4 space relative to the 'r' in return. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:262: return '<$element${cls == null ? '' : ' class="$cls"'}>' On 2013/11/29 12:03:13, kasperl wrote: > I'd compute the (cls == null ? '' : ' class ...') thing in a local variable to > make this more readable. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:336: // TODO (sigurdm) this script should be written in dart On 2013/11/29 12:03:13, kasperl wrote: > Usually we format these TODO comment like this: > > // TODO(sigurdm): Foo bar quux. > > with a colon and no space between TODO and ( and a terminating dot. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:339: function toggler(element) { return function(e) { On 2013/11/29 12:03:13, kasperl wrote: > Newline before return and more indentation. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:340: element.hidden = ! element.hidden; On 2013/11/29 12:03:13, kasperl wrote: > No space after !. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:341: console.log(element); On 2013/11/29 12:03:13, kasperl wrote: > Do you really want to log this to the console? Looks like debugging code. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:345: for(var i = 0; i < containers.length; i++) { On 2013/11/29 12:03:13, kasperl wrote: > for( -> for ( Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:350: container.nextElementSibling.hidden=true; On 2013/11/29 12:03:13, kasperl wrote: > Spaces around =. Done. https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/elements/visitor.dart (right): https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/elements/visitor.dart:21: abstract class ElementVisitor<R> { On 2013/11/29 12:03:13, kasperl wrote: > I thought we agreed on not visiting the ModelX classes but only the more > abstract interface types? What made you change your mind? I'd try to get rid of > all the visitFooBarX methods in this visitor. I changed my mind in https://codereview.chromium.org/96873002 https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/40001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1950: if (!generatedCode.containsKey(functionElement)) return null; On 2013/11/29 12:03:13, kasperl wrote: > Use ?:. Done. https://codereview.chromium.org/90713003/diff/40001/tests/compiler/dart2js/du... File tests/compiler/dart2js/dump_info_test.dart (right): https://codereview.chromium.org/90713003/diff/40001/tests/compiler/dart2js/du... tests/compiler/dart2js/dump_info_test.dart:22: () {} (); // TODO (sigurdm) empty closure, hack to avoid inlining On 2013/11/29 12:03:13, kasperl wrote: > See if you can use @NoInline() annotation and get rid of the TODO. This seems to be a bit harder - the annotation is only available internally to the compiler. https://codereview.chromium.org/90713003/diff/40001/tests/compiler/dart2js/du... tests/compiler/dart2js/dump_info_test.dart:37: Expect.stringEquals(mainlib.name, "main"); On 2013/11/29 12:03:13, kasperl wrote: > Expect.stringEquals expects the expected result as the first parameter, so you > should swap the two arguments for all these calls. Done.
Comments so far. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/compiler.dart (right): https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/compiler.dart:85: CodeBuffer codeOf(FunctionElement functionElement) => null; Please add new features at the end of this class definition. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:11: show Compiler, Put show on previous line and indent by four. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:38: class ElementInfo { I'd put a line between each member. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:44: // How to present this piece of information What does this comment apply to? Is it a TODO? https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:45: // As "element" or "code" Is this a sentence by itself or a continuation of the previous line? https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:47: final int size; // How many bytes does this take in the output Is this supposed to be a documentation comment? https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:59: class ProgramInfo { I'd put a line between each member. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:63: final int size; // How many bytes is the output Documentation comment? https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:76: Extra line. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:129: List inferredType = [new ElementInfo( I don't understand why an inferredType is a list. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1950: return generatedCode.containsKey(functionElement) I'm not too fond of looking up things twice in a table, and prefer this style: jsAst.Node code = generatedCode[functionElement]; return code == null ? null : jsAst.prettyPrint(code, compiler); https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart (right): https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1180: int sizeBefore = outputBuffers[outputUnit].length; I think you should cache the value outputBuffers[outputUnit] in a local variable. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1193: compiler.dumpInfoTask.codeSizeCounter This needs to be an abstract operation on dumpInfoTask, something like: compiler.dumpInfoTask.recordSize(library, sizeAfter - sizeBefore); https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/js_emitter/js_emitter.dart (right): https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/js_emitter/js_emitter.dart:61: import '../dump_info.dart' as dumpInfo; I find it problematic that the emitter needs to know about dumpInfo. I think it would be cleaner if the emitter told the backend (which then told dumpInfo).
Now also shows closures. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:11: show Compiler, On 2013/12/02 15:20:26, ahe wrote: > Put show on previous line and indent by four. Done. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:38: class ElementInfo { On 2013/12/02 15:20:26, ahe wrote: > I'd put a line between each member. Done. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:44: // How to present this piece of information On 2013/12/02 15:20:26, ahe wrote: > What does this comment apply to? Is it a TODO? Done. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:47: final int size; // How many bytes does this take in the output On 2013/12/02 15:20:26, ahe wrote: > Is this supposed to be a documentation comment? yes https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:59: class ProgramInfo { On 2013/12/02 15:20:26, ahe wrote: > I'd put a line between each member. Done. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:63: final int size; // How many bytes is the output On 2013/12/02 15:20:26, ahe wrote: > Documentation comment? Done. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:76: On 2013/12/02 15:20:26, ahe wrote: > Extra line. Done. https://codereview.chromium.org/90713003/diff/80001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/dump_info.dart:129: List inferredType = [new ElementInfo( On 2013/12/02 15:20:26, ahe wrote: > I don't understand why an inferredType is a list. Done.
Can't wait to see that in! https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/closure.dart (right): https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/closure.dart:78: List<FunctionElement> nestedClosures = new List<FunctionElement>(); What is this for a ClosureFieldElement? Please add a comment. https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:5: library dump_types; dump_types -> dump_info ? https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart (right): https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1166: int sizeBefore = outputBuffers[outputUnit].length; Why isn't the element visitor enough to know the full size of the library?
https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/closure.dart (right): https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/closure.dart:78: List<FunctionElement> nestedClosures = new List<FunctionElement>(); On 2013/12/05 09:29:28, ngeoffray wrote: > What is this for a ClosureFieldElement? Please add a comment. Actually nothing - I had misunderstood what a ClosureFieldElement is. I added a class FieldElement subclass of VariableElement - so only fields and functions have nestedClosures. https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:5: library dump_types; On 2013/12/05 09:29:28, ngeoffray wrote: > dump_types -> dump_info ? Done. https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart (right): https://codereview.chromium.org/90713003/diff/100001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1166: int sizeBefore = outputBuffers[outputUnit].length; On 2013/12/05 09:29:28, ngeoffray wrote: > Why isn't the element visitor enough to know the full size of the library? The code for the library looks like: ["", "simple.dart", , M, { foo1: function(a, c, f) { var i; for (i = 0; i < 10; ++i) a = C.JSString_methods.$add(a, new M.foo1_closure().call$1(10 + i)); return new M.foo1_closure0(); }, [[ snip ]] foo1__closure0: { "": "Closure;", call$0: function() { P.print($.b); } } }, 1] So there are some bytes that are "boiler-plate" - not directly belonging to any member
LGTM https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:69: class ElementInfoNode implements InfoNode { Add documentation to the class and its fields. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:127: class CodeInfoNode implements InfoNode { Add documentation to the class and its fields. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:146: class InferredInfoNode implements InfoNode { Add documentation to the class and its fields. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:161: '${span(esc(type.toString()), cls: 'type')} ')); Remove .toString() -- [type] is already a String. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:165: class ProgramInfo { Add documentation to the class and its fields. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:182: this.presentation, Indent this and the next lines to fit under `String this.name`. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:184: this.size, DateTime this.compilationMoment, Put `DateTime this.compilationMoment` on a separate line. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:185: this.compilationDuration, String this.dart2jsVersion}); Ditto for `String this.dart2jsVersion`. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:222: extra: element.canonicalUri.toString(), Maybe use '${element.canonicalUri}' instead? https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:245: if (inferredType == TypeMask) return null; Why do you compare to `TypeMask`, is that ever true? Should it be `if (inferredType == null) return null` and if so, why not include fields with no inferred type, can't we emitted code for these? https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:276: String modifiersString = element.modifiers.toString() == "" Change `element.modifiers.toString() == ""` to `element.modifiers.isEmpty` ? https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:278: : element.modifiers.toString()+" "; Change `element.modifiers.toString()+" "` to `'${element.modifiers} '` ? https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:279: if (!element.isResolved) return null; Check this as the first thing in the method and add a comment why such element are excluded. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:292: return null; Why exclude a class with no members. We might still emit code for it if it is used in type tests. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:309: String modifiersString = element.modifiers.toString(); Change `element.modifiers.toString()` to '${element.modifiers}' ? https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:314: stack.last.isFunction() || Indent `stack.last.isFunction()` to fit under `stack.last.isField()`. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:315: stack.last.isConstructor()) { Ditto. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:406: .where((library) => library != null)); Rename [library] to [info] -- it's not the library but its information that is null. https://codereview.chromium.org/90713003/diff/120001/tests/compiler/dart2js/d... File tests/compiler/dart2js/dump_info_test.dart (right): https://codereview.chromium.org/90713003/diff/120001/tests/compiler/dart2js/d... tests/compiler/dart2js/dump_info_test.dart:22: () {} (); // TODO (sigurdm) empty closure, hack to avoid inlining Nit: Fully format comments: TODO(sigurdm): Empty closure, hack to avoid inlining.
https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:69: class ElementInfoNode implements InfoNode { On 2013/12/10 10:40:53, Johnni Winther wrote: > Add documentation to the class and its fields. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:127: class CodeInfoNode implements InfoNode { On 2013/12/10 10:40:53, Johnni Winther wrote: > Add documentation to the class and its fields. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:146: class InferredInfoNode implements InfoNode { On 2013/12/10 10:40:53, Johnni Winther wrote: > Add documentation to the class and its fields. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:161: '${span(esc(type.toString()), cls: 'type')} ')); On 2013/12/10 10:40:53, Johnni Winther wrote: > Remove .toString() -- [type] is already a String. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:165: class ProgramInfo { On 2013/12/10 10:40:53, Johnni Winther wrote: > Add documentation to the class and its fields. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:182: this.presentation, On 2013/12/10 10:40:53, Johnni Winther wrote: > Indent this and the next lines to fit under `String this.name`. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:184: this.size, DateTime this.compilationMoment, On 2013/12/10 10:40:53, Johnni Winther wrote: > Put `DateTime this.compilationMoment` on a separate line. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:185: this.compilationDuration, String this.dart2jsVersion}); On 2013/12/10 10:40:53, Johnni Winther wrote: > Ditto for `String this.dart2jsVersion`. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:222: extra: element.canonicalUri.toString(), On 2013/12/10 10:40:53, Johnni Winther wrote: > Maybe use '${element.canonicalUri}' instead? Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:245: if (inferredType == TypeMask) return null; On 2013/12/10 10:40:53, Johnni Winther wrote: > Why do you compare to `TypeMask`, is that ever true? Should it be `if > (inferredType == null) return null` and if so, why not include fields with no > inferred type, can't we emitted code for these? Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:276: String modifiersString = element.modifiers.toString() == "" On 2013/12/10 10:40:53, Johnni Winther wrote: > Change `element.modifiers.toString() == ""` to `element.modifiers.isEmpty` ? Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:278: : element.modifiers.toString()+" "; On 2013/12/10 10:40:53, Johnni Winther wrote: > Change `element.modifiers.toString()+" "` to `'${element.modifiers} '` ? Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:279: if (!element.isResolved) return null; On 2013/12/10 10:40:53, Johnni Winther wrote: > Check this as the first thing in the method and add a comment why such element > are excluded. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:292: return null; On 2013/12/10 10:40:53, Johnni Winther wrote: > Why exclude a class with no members. We might still emit code for it if it is > used in type tests. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:309: String modifiersString = element.modifiers.toString(); On 2013/12/10 10:40:53, Johnni Winther wrote: > Change `element.modifiers.toString()` to '${element.modifiers}' ? Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:314: stack.last.isFunction() || On 2013/12/10 10:40:53, Johnni Winther wrote: > Indent `stack.last.isFunction()` to fit under `stack.last.isField()`. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:315: stack.last.isConstructor()) { On 2013/12/10 10:40:53, Johnni Winther wrote: > Ditto. Done. https://codereview.chromium.org/90713003/diff/120001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:406: .where((library) => library != null)); On 2013/12/10 10:40:53, Johnni Winther wrote: > Rename [library] to [info] -- it's not the library but its information that is > null. Done. https://codereview.chromium.org/90713003/diff/120001/tests/compiler/dart2js/d... File tests/compiler/dart2js/dump_info_test.dart (right): https://codereview.chromium.org/90713003/diff/120001/tests/compiler/dart2js/d... tests/compiler/dart2js/dump_info_test.dart:22: () {} (); // TODO (sigurdm) empty closure, hack to avoid inlining On 2013/12/10 10:40:53, Johnni Winther wrote: > Nit: Fully format comments: > TODO(sigurdm): Empty closure, hack to avoid inlining. Done.
https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:61: /// Instances represent a piece of information about a part the program. `Instances represent` -> `An [InfoNode] holds` ? https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:70: /// Instances represent information about an [Element] `Instance represent` -> `An [ElementInfoNode] holds` ? https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:75: /// The kind of the [Element] represented. This is for presenting to the `This is for presenting` -> `This is presented` ? https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:95: /// [Element], and about its members. `, and about` -> `and` https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:139: /// Instances represent information about a piece of code. An [CodeInfoNode] holds .... https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:141: final String description; Comment here? https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:159: /// Instances represent information inferred about an [Element] such as a type `type` -> `type information` https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:189: /// How many bytes does the whole program take up. Maybe `The size of the whole program in bytes.` to avoid the question-like sentence.
https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:61: /// Instances represent a piece of information about a part the program. On 2013/12/10 12:05:19, Johnni Winther wrote: > `Instances represent` -> `An [InfoNode] holds` ? Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:70: /// Instances represent information about an [Element] On 2013/12/10 12:05:19, Johnni Winther wrote: > `Instance represent` -> `An [ElementInfoNode] holds` ? Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:75: /// The kind of the [Element] represented. This is for presenting to the On 2013/12/10 12:05:19, Johnni Winther wrote: > `This is for presenting` -> `This is presented` ? Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:95: /// [Element], and about its members. On 2013/12/10 12:05:19, Johnni Winther wrote: > `, and about` -> `and` Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:139: /// Instances represent information about a piece of code. On 2013/12/10 12:05:19, Johnni Winther wrote: > An [CodeInfoNode] holds .... Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:141: final String description; On 2013/12/10 12:05:19, Johnni Winther wrote: > Comment here? Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:159: /// Instances represent information inferred about an [Element] such as a type On 2013/12/10 12:05:19, Johnni Winther wrote: > `type` -> `type information` Done. https://codereview.chromium.org/90713003/diff/130001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:189: /// How many bytes does the whole program take up. On 2013/12/10 12:05:19, Johnni Winther wrote: > Maybe `The size of the whole program in bytes.` to avoid the question-like > sentence. Done.
Message was sent while issue was closed.
Committed patchset #8 manually as r31060 (presubmit successful).
Message was sent while issue was closed.
dbc https://codereview.chromium.org/90713003/diff/170001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/170001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:196: /// The time the compilation took to complite. complete https://codereview.chromium.org/90713003/diff/170001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:471: '${info.compilationDuration/1000000} seconds')); the compilationDuration is stored in ticks. A tick is not necessarily 1/1000000s. I think it would be better to store a Duration in the compilationDuration and then ask for duration.inSeconds (or inMilliseconds).
Message was sent while issue was closed.
Followed up in https://codereview.chromium.org/112813002 https://codereview.chromium.org/90713003/diff/170001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/implementation/dump_info.dart (right): https://codereview.chromium.org/90713003/diff/170001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:196: /// The time the compilation took to complite. On 2013/12/11 13:04:19, floitsch wrote: > complete Done. https://codereview.chromium.org/90713003/diff/170001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/implementation/dump_info.dart:471: '${info.compilationDuration/1000000} seconds')); Good idea |
