Record dependency information to implement first version of dependency

Record dependency information to implement first version of dependency
graph segmentation for lazy loading.

What this means is that a lazily loaded library can depend on stuff
in, for example, dart:core without including this in the initial
download.

Committed: https://code.google.com/p/dart/source/detail?r=19793

[ahe, 2013-03-06 09:55:41]

I'll probably follow up with a few minor changes to this CL as I discover
call-sites where I forgot to an extra argument for dependency tracking.

Also, I haven't carefully reviewed the code myself, so there might be a few
brain farts left, I apologize in advance.

With those caveats, please do review this. It should be ready.

[kasperl, 2013-03-06 10:26:51]

Exciting! First round of comments -- will dive into more details shortly.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:40: final
Set<Element> deferredElements = new LinkedHashSet<Element>();
Is there a reason this needs to be linked when the deferredLibraries set does
not?

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:46: String get
name => 'Lazy';
Deferred?

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:87:
Map<LibraryElement, Set<Element>> deferredElements =
Not sure the shadowing (field/local) here helps the understandability.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:91:
mainApp.forEachLocalMember((Element e) {
Maybe add a comment here that briefly outlines what you're doing while running
through the local members of the main app?

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:94:
deferredElements.putIfAbsent(dependency.getLibrary(),
I think this would be easier to read if you pulled the result of calling
putIfAbsent into a local variable.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:114: 
Too many newlines.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:116: // Pseudo
code to be implemented:
Not sure I understand how to review this based on this comment.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:122:
reverseMap.putIfAbsent(e,
You an extra local here too?

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:127: // Now
compute the output files based on the lists in reverseMap.
Change to a TODO or add the code?

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:136: //
TODO(ahe): This doesn't work. I need to record classes and
Hmm. The code seems to deal with classes, statics, and top-level elements?

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:157: void
computeTransitiveClosure(Set<Element> elements) {
The method names makes it sound like you'll be returning a new set so maybe
something like:

   void addTransitiveClosureTo(Set elements)

would be better?

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js/d...
File dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart
(right):

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js/d...
dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart:63:
compiler.deferredLoadTask.deferredElements.where((e) => e.isClass())
4 space indent.

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js_e...
File dart/tests/compiler/dart2js_extra/deferred/deferred_class_library.dart
(right):

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js_e...
dart/tests/compiler/dart2js_extra/deferred/deferred_class_library.dart:14: new
Expando();
Remove this again? Isn't this covered by the test case you added.

[ahe, 2013-03-06 16:01:41]

Thank you, Kasper.  I have addressed your comments and made a few other
adjustments based on issues found during testing.  I also reduced the number of
places I use compiler.globalDependencies.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:40: final
Set<Element> deferredElements = new LinkedHashSet<Element>();
On 2013/03/06 10:26:51, kasperl wrote:
> Is there a reason this needs to be linked when the deferredLibraries set does
> not?

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:46: String get
name => 'Lazy';
On 2013/03/06 10:26:51, kasperl wrote:
> Deferred?

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:87:
Map<LibraryElement, Set<Element>> deferredElements =
On 2013/03/06 10:26:51, kasperl wrote:
> Not sure the shadowing (field/local) here helps the understandability.

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:91:
mainApp.forEachLocalMember((Element e) {
On 2013/03/06 10:26:51, kasperl wrote:
> Maybe add a comment here that briefly outlines what you're doing while running
> through the local members of the main app?

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:94:
deferredElements.putIfAbsent(dependency.getLibrary(),
On 2013/03/06 10:26:51, kasperl wrote:
> I think this would be easier to read if you pulled the result of calling
> putIfAbsent into a local variable.

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:114: 
On 2013/03/06 10:26:51, kasperl wrote:
> Too many newlines.

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:116: // Pseudo
code to be implemented:
On 2013/03/06 10:26:51, kasperl wrote:
> Not sure I understand how to review this based on this comment.

I updated the comment to be a TODO. Hope it is clearer now.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:122:
reverseMap.putIfAbsent(e,
On 2013/03/06 10:26:51, kasperl wrote:
> You an extra local here too?

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:127: // Now
compute the output files based on the lists in reverseMap.
On 2013/03/06 10:26:51, kasperl wrote:
> Change to a TODO or add the code?

Done.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:136: //
TODO(ahe): This doesn't work. I need to record classes and
On 2013/03/06 10:26:51, kasperl wrote:
> Hmm. The code seems to deal with classes, statics, and top-level elements?

This was an old note to myself I forgot to remove.

https://codereview.chromium.org/12525007/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:157: void
computeTransitiveClosure(Set<Element> elements) {
On 2013/03/06 10:26:51, kasperl wrote:
> The method names makes it sound like you'll be returning a new set so maybe
> something like:
> 
>    void addTransitiveClosureTo(Set elements)
> 
> would be better?

Done.

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js/d...
File dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart
(right):

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js/d...
dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart:63:
compiler.deferredLoadTask.deferredElements.where((e) => e.isClass())
On 2013/03/06 10:26:51, kasperl wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js_e...
File dart/tests/compiler/dart2js_extra/deferred/deferred_class_library.dart
(right):

https://codereview.chromium.org/12525007/diff/1/dart/tests/compiler/dart2js_e...
dart/tests/compiler/dart2js_extra/deferred/deferred_class_library.dart:14: new
Expando();
On 2013/03/06 10:26:51, kasperl wrote:
> Remove this again? Isn't this covered by the test case you added.

Done.

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:286:
(x1, x2, x3, x4, x5) => null,
Should disappear when I merge with top of tree.

[kasperl, 2013-03-06 20:39:43]

LGTM.

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart
(right):

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:855: //
This is a true global dependency.
Extend comment to explain why? You could also add a special method in the
enqueuer for registering these "true global dependencies". That seems better
than have comments in the code.

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1194:
GenericType supertype = subclass.supertype;
Already changed to InterfaceType. Should go away when you merge from the repo.

https://codereview.chromium.org/12525007/diff/5002/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart
(right):

https://codereview.chromium.org/12525007/diff/5002/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart:51:
Compiler compiler = new Compiler(provider.readStringFromUri,
This looks pretty nice. I think we can rewrite some of our uses of the mock
compiler to use something like this instead and simplify the mock compiler
again.

[ahe, 2013-03-06 20:54:02]

Responding to comments, no new patch set.

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart
(right):

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:855: //
This is a true global dependency.
On 2013/03/06 20:39:43, kasperl wrote:
> Extend comment to explain why? You could also add a special method in the
> enqueuer for registering these "true global dependencies". That seems better
> than have comments in the code.

Actually, I need to think more about this.  How many times are we calling
registerInstantiatedClass for the same class?  I think we only call it once, so
I should remove the "elements" parameter from registerInstantiatedClass as well.

https://codereview.chromium.org/12525007/diff/5002/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart
(right):

https://codereview.chromium.org/12525007/diff/5002/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/deferred_load_graph_segmentation_test.dart:51:
Compiler compiler = new Compiler(provider.readStringFromUri,
On 2013/03/06 20:39:43, kasperl wrote:
> This looks pretty nice. I think we can rewrite some of our uses of the mock
> compiler to use something like this instead and simplify the mock compiler
> again.

That was in the back of my mind when I devised this test, also, this wouldn't
have been possible if Johnni hadn't built source_file_provider.dart.

[ahe, 2013-03-07 10:24:13]

PTAL at patch set #4.

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12525007/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:479: // TODO(ahe):
This doesn't seem to be necessary. Why is that?
It is necessary, but right now the implementation of dart:async pulls in
dart:isolate so is automatically included in the eagerly loaded elements.

[kasperl, 2013-03-07 10:31:37]

Still LGTM -- actually it looks even better.

[ahe, 2013-03-07 11:00:47]

After I merged with top of tree, the test is no longer passing because I see
both Expando origin and Expando patch in
compiler.deferredLoadTask.allDeferredElements.

I need to figure out what is the right thing to do.

[ngeoffray, 2013-03-07 11:21:14]

LGTM

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:8: import
'dart:collection';
show LinkedHashSet?

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:111: } else if
(mainApp != dependency.getLibrary()) {
Nitpicking, but I would reverse the operators here. It reads nicer.

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:158: element =
element.getEnclosingClass();
This looks magic. Is it to get the fields? Please add a comment. Maybe the
cleaner way to get allElementsResolvedFrom for the fields too?

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:171: }
Why can't you end up with a member here? Should you assert it?

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:100: // We only tell
the backend once that [cls] was instantiated, so
Add an assert in the backend?

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:436: if (owner ==
null) {
What is this?

[ahe, 2013-03-11 12:28:01]

PTAL at patch set 6 + 7.

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:8: import
'dart:collection';
On 2013/03/07 11:21:14, ngeoffray wrote:
> show LinkedHashSet?

Done.

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:111: } else if
(mainApp != dependency.getLibrary()) {
On 2013/03/07 11:21:14, ngeoffray wrote:
> Nitpicking, but I would reverse the operators here. It reads nicer.

Done.

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:158: element =
element.getEnclosingClass();
On 2013/03/07 11:21:14, ngeoffray wrote:
> This looks magic. Is it to get the fields? Please add a comment. Maybe the
> cleaner way to get allElementsResolvedFrom for the fields too?

I've added a comment.

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:171: }
On 2013/03/07 11:21:14, ngeoffray wrote:
> Why can't you end up with a member here? Should you assert it?

I can end up with a member, but that is not relevant.  I only care about classes
and static methods.  I've added more comments.

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:100: // We only tell
the backend once that [cls] was instantiated, so
On 2013/03/07 11:21:14, ngeoffray wrote:
> Add an assert in the backend?

Why, and how would you implement this assertion?

https://codereview.chromium.org/12525007/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:436: if (owner ==
null) {
On 2013/03/07 11:21:14, ngeoffray wrote:
> What is this?

Certain elements do not have an owner.  I'm calling this method on more elements
than before, so it has to be a little be more robust.

[ahe, 2013-03-11 14:27:17]

Committed patchset #9 manually as r19793 (presubmit successful).