dart2js send stats, includes:

dart2js send stats, includes:
- initial visitor to collecting data about sends (incomplete)
- logic to emit data in a json summary file.
- logic to print summary information on the command-line
- logic to display some of the data in a web UI.

BUG=
R=johnniwinther@google.com

Committed: https://github.com/dart-lang/sdk/commit/93f61f383e4d56ce1aefc75944328719cc77f2ab

[Siggi Cherem (dart-lang), 2015-07-06 19:59:54]

sigmund@google.com changed reviewers:
+ herhut@google.com, johnniwinther@google.com

[Siggi Cherem (dart-lang), 2015-07-06 19:59:55]

This is getting big, so I rather start submitting what I have. It's pretty half
baked though - Lots of the classification is missing, we don't yet have any data
from the inferrer, and I'd like to eventually merge this with dump-info too, so
lots to do. 

Anything under `tool/stats/` is for visualization, so it's not as important as
the logic itself that is computing information about sends.

Harry and Stephen are on vacation, so this is mainly for Johnni and Stephan.

Looking forward to your comments!

[Johnni Winther, 2015-07-13 19:21:44]

lgtm

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/analysis_result.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/analysis_result.dart:14: enum boolish { yes, no,
maybe }
'boolish' -> 'Boolish'

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/builder.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:165: measurements.record(Metric.send,
span.begin, span.end);
Add a comment that you count both the get and the set operation.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:179: 
Add comments for these. For instance what is the difference between a 'dynamic'
and 'virtual' send and why do we know some to be mono-/polymorphic?

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:521: handleIndex(node);
It looks a bit weird that you (don't have to) discriminate between the get and
set. Here and throughout the visitor.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:970: // Statically known "no such
method" sends
Indent.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:1484: handleNSMError(node);
This weird case might actually included both the operator and setter:

class A {
  noSuchMethod(Invocation i) => 42;
}

class B extends A {
  foo() {
    super.bar += 87;
  }
}

Similar below.

Also: Shouldn't it be 'handleNSMSuper(node)' instead of 'handleNSMError(node)' ?

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:1587: Element element,
AssignmentOperator operator, Node rhs, T arg) {
Here the getter _does_ exists but not the setter, i.e. we should have

handleSuper(node);
handleOperator(node);
handleNSMSuper(node);

Similar below.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/stats.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:58: /// the systems libraries, loose
libraries, and to create a separate a group per
'a separate a group' -> 'a separate group'

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:74: final List<String> classes = [];
What is this used for?

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:291: /// fucntion, a library, a package,
or an entire program.
'fucntion' -> 'function'

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:419: /// Color-highighted string used
mainly to debug invariants.
'highighted' -> 'highlighted'

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:438: sb.write('');
Note that colors are not supported on Windows by default. Maybe use colors
conditionally on the --enable-diagnostic-colors option.

And maybe use src/colors.dart.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/trusted_types_analysis_result.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/trusted_types_analysis_result.dart:49: for (var cls
in world.subtypesOf(type.element)) {
I have just removed ClassWorld.subtypesOf because the implementation was
effectively ClassWorld.strictSubtypesOf and no one need the non-strict version.

In general I want to move towards high level queries on [ClassWorld] so that we
can make optimized implementation instead of the brute-force implementation at
callsites.

For now, reinsert ClassWorld.subtypesOf if needed.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:368: // TODO(sigmund): split into separate CL.
See dartbug.com/23664
Ahh, yes, the bug and its fix.

[Siggi Cherem (dart-lang), 2015-09-29 01:39:33]

sigmund@google.com changed reviewers:
+ het@google.com

[Siggi Cherem (dart-lang), 2015-09-29 01:39:34]

I finally got back to this, I made a big change to integrate the logic with
dart2js's info. So:
- the model to represent the metrics moved to the dart2js_info package (created
CL here: https://codereview.chromium.org/1372333002/)
- updated logic here to make it part of dump info (as long as a special -D flag
is passed to the vm).

To make diffing easier, I kept the folder name stats and the filename
stats/builder.dart, but I think we should rename both either before submitting
or soon afterwards, maybe:
- stats -> info  (and move dump_info there)
- builder -> send_metrics_collector.dart

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/analysis_result.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/analysis_result.dart:14: enum boolish { yes, no,
maybe }
On 2015/07/13 19:21:43, Johnni Winther wrote:
> 'boolish' -> 'Boolish'

Done.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/builder.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:165: measurements.record(Metric.send,
span.begin, span.end);
On 2015/07/13 19:21:44, Johnni Winther wrote:
> Add a comment that you count both the get and the set operation.

Done.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:179: 
On 2015/07/13 19:21:43, Johnni Winther wrote:
> Add comments for these. For instance what is the difference between a
'dynamic'
> and 'virtual' send and why do we know some to be mono-/polymorphic?

Done - I made the comment a bit short here because there is a general
explanation in the Metric description.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:521: handleIndex(node);
On 2015/07/13 19:21:43, Johnni Winther wrote:
> It looks a bit weird that you (don't have to) discriminate between the get and
> set. Here and throughout the visitor.

Yeah - this might change as right now handleIndex is not implemented (it's
marked as dynamic) - I might need the distinction once I measure it correctly.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:970: // Statically known "no such
method" sends
On 2015/07/13 19:21:43, Johnni Winther wrote:
> Indent.

Done.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:1484: handleNSMError(node);
On 2015/07/13 19:21:43, Johnni Winther wrote:
> This weird case might actually included both the operator and setter:
> 
> class A {
>   noSuchMethod(Invocation i) => 42;
> }
> 
> class B extends A {
>   foo() {
>     super.bar += 87;
>   }
> }
> 
> Similar below.
> 
> Also: Shouldn't it be 'handleNSMSuper(node)' instead of 'handleNSMError(node)'
?

Good catch, you are correct. I thought in this scenario we would reach this
visit method (I assumed we wouldn't say that `bar` is unresolved if super had a
nsm).

I made the change to match the current semantics. As you said, we should use
NSMSuper and count the operator + assignment (although if we know that the type
has no nSM, we should reduce the count to 1 and not 3 ops, but we can leave that
for later)

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/builder.dart:1587: Element element,
AssignmentOperator operator, Node rhs, T arg) {
On 2015/07/13 19:21:43, Johnni Winther wrote:
> Here the getter _does_ exists but not the setter, i.e. we should have
> 
> handleSuper(node);
> handleOperator(node);
> handleNSMSuper(node);
> 
> Similar below.

good point. done

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/stats.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:58: /// the systems libraries, loose
libraries, and to create a separate a group per
On 2015/07/13 19:21:44, Johnni Winther wrote:
> 'a separate a group' -> 'a separate group'

Ack. (this went away after refactoring)

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:74: final List<String> classes = [];
On 2015/07/13 19:21:44, Johnni Winther wrote:
> What is this used for?

it was a placeholder for the future. now that I'm integrating this with the rest
of dart2js's info, this disappeared.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/stats.dart:291: /// fucntion, a library, a package,
or an entire program.
On 2015/07/13 19:21:44, Johnni Winther wrote:
> 'fucntion' -> 'function'

Done.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
File pkg/compiler/lib/src/stats/trusted_types_analysis_result.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/stats/...
pkg/compiler/lib/src/stats/trusted_types_analysis_result.dart:49: for (var cls
in world.subtypesOf(type.element)) {
On 2015/07/13 19:21:44, Johnni Winther wrote:
> I have just removed ClassWorld.subtypesOf because the implementation was
> effectively ClassWorld.strictSubtypesOf and no one need the non-strict
version.
> 
> In general I want to move towards high level queries on [ClassWorld] so that
we
> can make optimized implementation instead of the brute-force implementation at
> callsites.
> 
> For now, reinsert ClassWorld.subtypesOf if needed.

thanks, I re-added the API.

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/1220043005/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:368: // TODO(sigmund): split into separate CL.
See dartbug.com/23664
On 2015/07/13 19:21:44, Johnni Winther wrote:
> Ahh, yes, the bug and its fix.

:)

[Siggi Cherem (dart-lang), 2015-10-02 18:10:46]

Committed patchset #5 (id:80001) manually as
93f61f383e4d56ce1aefc75944328719cc77f2ab (presubmit successful).