Start implementing IncrementalClassHierarchy.

Start implementing IncrementalClassHierarchy.

Only getClassDepth() for now.

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/65c1f5f8ab15ad1ce96b208ce1a152da7299246c

[Paul Berry, 2017-06-01 21:39:03]

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:19: // TODO: implement
classes
Change the TODOs in this file to "TODO(scheglov)"

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:20: return null;
I'd recommend replacing this line (and the other "return null;" lines) with
"throw new UnimplementedError();".  Less likely to result in confusing failures.

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:107: var info = _info[node];
I think a more performant implementation of this function would be:

_ClassInfo _getInfo(Class node) =>
    _info.putIfAbsent(node, () => new _ClassInfo(node));

(on a miss, it requires just one hashtable lookup rather than two).

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:121: /// to [Object].
", or -1 if the depth has not been computed yet."

[scheglov, 2017-06-01 21:50:36]

PTAL

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:19: // TODO: implement
classes
On 2017/06/01 21:39:03, Paul Berry wrote:
> Change the TODOs in this file to "TODO(scheglov)"

Done.

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:20: return null;
On 2017/06/01 21:39:03, Paul Berry wrote:
> I'd recommend replacing this line (and the other "return null;" lines) with
> "throw new UnimplementedError();".  Less likely to result in confusing
failures.

Done.

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:107: var info = _info[node];
On 2017/06/01 21:39:03, Paul Berry wrote:
> I think a more performant implementation of this function would be:
> 
> _ClassInfo _getInfo(Class node) =>
>     _info.putIfAbsent(node, () => new _ClassInfo(node));
> 
> (on a miss, it requires just one hashtable lookup rather than two).

Done.

https://codereview.chromium.org/2922533002/diff/1/pkg/kernel/lib/src/incremen...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:121: /// to [Object].
On 2017/06/01 21:39:03, Paul Berry wrote:
> ", or -1 if the depth has not been computed yet."

Done.

[Paul Berry, 2017-06-01 21:59:04]

lgtm

[scheglov, 2017-06-01 22:01:58]

Description was changed from

==========
Start implementing IncrementalClassHierarchy.

Only getClassDepth() for now.

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Start implementing IncrementalClassHierarchy.

Only getClassDepth() for now.

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/65c1f5f8ab15ad1ce96b208ce1a152da7299246c
==========

[scheglov, 2017-06-01 22:01:59]

Committed patchset #2 (id:20001) manually as
65c1f5f8ab15ad1ce96b208ce1a152da7299246c (presubmit successful).

[ahe, 2017-06-02 11:34:48]

lgtm

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:14: /// It is filled lazily
as the client requests information about classes.
What are your plans for the incremental part of this?

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:42: superDepth =
max(superDepth, getClassDepth(node.supertype.classNode));
Any concerns about recursion here?

[Paul Berry, 2017-06-02 12:25:49]

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:42: superDepth =
max(superDepth, getClassDepth(node.supertype.classNode));
On 2017/06/02 11:34:48, ahe wrote:
> Any concerns about recursion here?

FWIW the topological sort in ClosedWorldClassHierarchy has the same potential
issue, and AFAIK it has not caused any problems yet.

[ahe, 2017-06-02 12:29:04]

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:42: superDepth =
max(superDepth, getClassDepth(node.supertype.classNode));
On 2017/06/02 12:25:48, Paul Berry wrote:
> On 2017/06/02 11:34:48, ahe wrote:
> > Any concerns about recursion here?
> 
> FWIW the topological sort in ClosedWorldClassHierarchy has the same potential
> issue, and AFAIK it has not caused any problems yet.

This situation might be slightly different. However, if you have called
SourceLoader.checkSemantics first, you should be fine.

[scheglov, 2017-06-02 15:36:07]

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:14: /// It is filled lazily
as the client requests information about classes.
On 2017/06/02 11:34:48, ahe wrote:
> What are your plans for the incremental part of this?

I think we can use single instance of IncrementalClassHierarchy to sequentially
analyze all strong connected components of libraries. So, every new class we
will request information for is "increment" to already known information about
its superclasses defined in previous components.

For mixin processing, we might want to clear information about a class when we
make changes to its structure, e.g. copy methods into it.

[ahe, 2017-06-02 15:43:46]

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
File pkg/kernel/lib/src/incremental_class_hierarchy.dart (right):

https://codereview.chromium.org/2922533002/diff/20001/pkg/kernel/lib/src/incr...
pkg/kernel/lib/src/incremental_class_hierarchy.dart:14: /// It is filled lazily
as the client requests information about classes.
On 2017/06/02 15:36:07, scheglov wrote:
> On 2017/06/02 11:34:48, ahe wrote:
> > What are your plans for the incremental part of this?
> 
> I think we can use single instance of IncrementalClassHierarchy to
sequentially
> analyze all strong connected components of libraries. So, every new class we
> will request information for is "increment" to already known information about
> its superclasses defined in previous components.
> 
> For mixin processing, we might want to clear information about a class when we
> make changes to its structure, e.g. copy methods into it.

Thank you for explaining.