Tool for visualizing type inference graph.

Tool for visualizing type inference graph.

BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/408e38e82fe4b08c700578953708225e18ef18d2

Reverted: https://github.com/dart-lang/sdk/commit/dce808a44852a31899de1982229b878d3d077a24

Committed: https://github.com/dart-lang/sdk/commit/35a984ac7f5bc86c0f705b7b54b2b4dec7399b59

[asgerf, 2016-03-03 11:40:38]

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

[asgerf, 2016-03-03 11:40:39]

We really ought to polish and commit this stuff instead of having it lying
around, so here is mine.

If you would rather commit yours, that's fine too.

One thing that caught my eye is the large amount of phi and narrow nodes in the
graph. E.g. see the graph for Sort._dualPivotQuicksort.

[asgerf, 2016-03-11 14:27:54]

Ping.

[kevmoo, 2016-03-11 16:47:03]

On 2016/03/11 14:27:54, asgerf wrote:
> Ping.

FYI: I have code that wraps a graphvis output in html – it even opens the
browser for you: https://github.com/kevmoo/pubviz

[Siggi Cherem (dart-lang), 2016-03-11 19:06:15]

lgtm!

This is a million times better than my hackish version I had. In case you are
curious, here is a patch with the dump logic: 

https://codereview.chromium.org/1783083004/diff/20001/pkg/compiler/lib/src/in...

In summary: my nodes were much simpler, I had a single graph for the entire
program, I also removed the "TypeInformation" string from the nodes, and I
included comments in the output dot file to add details that would make the
graph too cluttered.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_dump.dart (right):

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:42: Set<TypeInformation> copy
= node.assignments.toSet();
I can't remember why, but I found sometimes useful to also print the users in my
graph. I initially expected them to be just the reverse of the assignments, but
if I recall correctly it was not always identical. Maybe it was a bug at the
time or I'm misremembering though.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:88:
parts.add(element.library?.libraryName);
since library name is optional and can be duplicated, this can be a source of
collisions. Maybe we should use a sanitized version of the Uri?

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:171: return getFreshId();
fwiw - I used the node hashCode for this when I built mine. It might not make
sense here because of the duplicates you create below (and also because you are
doing a single function, in which case numbers tend to be small instead)

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:173: return '$id';
consider making the id more sophisticated to add details: like the type of the
node. It can make it easier to read the .dot file if you wanted to scan for
edges quickly.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:201: // for every use to
enhance readability.
nice

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:250: /// based on
[info.node].
info.node => node.assignments?

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:265: append('$id
[shape=record,label="$label",$style]');
ohh, records are neat :)

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:274: // TODO(asgerf): Could
there be edges that were added and then removed?
yes, this can happen and I think we can detect some of that if we copy edges one
more time before doing tracing (my understanding is that we add edges as we run
the first phase, do tracing, remove edges where we know better now that we have
the tracing results, then add edges again as we run the second phase.)

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:368: addNode(info, 'Parameter
${info.element?.name ?? ''}');
just to double check: this means we will see incoming edges into parameters that
come from other methods, correct? (and that's when the "from" portion of the
label gets added)

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_nodes.dart (right):

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_nodes.dart:72: TypedElement get
contextMember => context == null ? null : context.element;
was this just wrong before? should we make this change separately?

[asgerf, 2016-03-14 12:47:45]

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_dump.dart (right):

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:42: Set<TypeInformation> copy
= node.assignments.toSet();
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> I can't remember why, but I found sometimes useful to also print the users in
my
> graph. I initially expected them to be just the reverse of the assignments,
but
> if I recall correctly it was not always identical. Maybe it was a bug at the
> time or I'm misremembering though.

I believe the assignments set is only used for nodes that take an unordered
collection of nodes as input.  The receiver and arguments to an invocation, for
instance, do not count as assignments, but they still count as uses.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:88:
parts.add(element.library?.libraryName);
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> since library name is optional and can be duplicated, this can be a source of
> collisions. Maybe we should use a sanitized version of the Uri?

I tried that for a while, but found that it's too hard to sanitize the URI and
guarantee uniqueness while maintaining readability, typeability,
tab-completability etc.

I added explicit disambiguation of the filename to ensure we don't overwrite a
file we just wrote.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:171: return getFreshId();
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> fwiw - I used the node hashCode for this when I built mine. It might not make
> sense here because of the duplicates you create below (and also because you
are
> doing a single function, in which case numbers tend to be small instead)

Acknowledged.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:173: return '$id';
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> consider making the id more sophisticated to add details: like the type of the
> node. It can make it easier to read the .dot file if you wanted to scan for
> edges quickly.

Sorry, I don't really understand the use case here.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:250: /// based on
[info.node].
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> info.node => node.assignments?

Yep, thanks.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:265: append('$id
[shape=record,label="$label",$style]');
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> ohh, records are neat :)

Oh yeah :)

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:274: // TODO(asgerf): Could
there be edges that were added and then removed?
On 2016/03/11 19:06:14, Siggi Cherem (dart-lang) wrote:
> yes, this can happen and I think we can detect some of that if we copy edges
one
> more time before doing tracing (my understanding is that we add edges as we
run
> the first phase, do tracing, remove edges where we know better now that we
have
> the tracing results, then add edges again as we run the second phase.)

Thanks. Added orange edges for these.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_dump.dart:368: addNode(info, 'Parameter
${info.element?.name ?? ''}');
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> just to double check: this means we will see incoming edges into parameters
that
> come from other methods, correct? (and that's when the "from" portion of the
> label gets added)

Yes.

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_nodes.dart (right):

https://codereview.chromium.org/1759653003/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_nodes.dart:72: TypedElement get
contextMember => context == null ? null : context.element;
On 2016/03/11 19:06:15, Siggi Cherem (dart-lang) wrote:
> was this just wrong before? should we make this change separately?

It was just wrong, but wasn't actually causing any problems until I started
calling it for every node in the graph.  I'll land it separately.

[asgerf, 2016-03-14 12:57:58]

Description was changed from

==========
Tool for visualizing type inference graph.

BUG=
==========

to

==========
Tool for visualizing type inference graph.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/408e38e82fe4b08c700578953708225e18ef18d2
==========

[asgerf, 2016-03-14 12:57:59]

Committed patchset #5 (id:80001) manually as
408e38e82fe4b08c700578953708225e18ef18d2 (presubmit successful).

[asgerf, 2016-03-14 13:25:05]

Description was changed from

==========
Tool for visualizing type inference graph.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/408e38e82fe4b08c700578953708225e18ef18d2
==========

to

==========
Tool for visualizing type inference graph.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/408e38e82fe4b08c700578953708225e18ef18d2

Reverted:
https://github.com/dart-lang/sdk/commit/dce808a44852a31899de1982229b878d3d077a24
==========

[asgerf, 2016-03-14 14:15:27]

Description was changed from

==========
Tool for visualizing type inference graph.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/408e38e82fe4b08c700578953708225e18ef18d2

Reverted:
https://github.com/dart-lang/sdk/commit/dce808a44852a31899de1982229b878d3d077a24
==========

to

==========
Tool for visualizing type inference graph.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/408e38e82fe4b08c700578953708225e18ef18d2

Reverted:
https://github.com/dart-lang/sdk/commit/dce808a44852a31899de1982229b878d3d077a24

Committed:
https://github.com/dart-lang/sdk/commit/35a984ac7f5bc86c0f705b7b54b2b4dec7399b59
==========

[asgerf, 2016-03-14 14:15:29]

Committed patchset #7 (id:120001) manually as
35a984ac7f5bc86c0f705b7b54b2b4dec7399b59 (presubmit successful).