Load deferred chunks in the right order.

Load deferred chunks in the right order.

A chunk can depend on classes in shared chunks. Therefore sort they are
sorted, and loaded sequentially.

BUG= dartbug.com/17890
R=floitsch@google.com

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

Reverted: r34702

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

[floitsch, 2014-04-02 09:32:48]

LGTM with comments.

Also: it's easy to keep the boolean return value: forEach(...).then((_) => true)

However it's actually hard to make sure to guarantee the correct behavior:

var f = lib.loadLibrary()
var f2 = lib.loadLibrary();

f.then(print);  // prints true.
f2.then(print);  // f2 is probably the same as f. -> "true".

We could make "f" be true, and f2 be a different future. But that feels
dangerous, too.

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

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:617: // Sort the
outputunits in descending order of the number of imports they
output units

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:618: // include.
Explain why this is necessary and enough.

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3223: return new Future(() => null);
new Future.value(null);

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3228: index < JS('int',
'\$.libraries_to_load[#].length', loadId);
Move the JS outside the loop. This way we can have a one-line for.

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3241: hunkNames.map((hunkName) =>
_loadHunk(hunkName, uri));
indentation.

[sigurdm, 2014-04-02 11:25:30]

> Also: it's easy to keep the boolean return value: forEach(...).then((_) =>
true)

Yes that would actually work - I was thinking we needed to answer true iff all
the chunks were not loaded already (a deferred import can depend on only a
shared chunk), but each library might want to answer true the first time
loadLibrary is called...

Anyhow - as you point out, we would like to just return the same future.
Also I think the boolean value has very marginal usability.

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

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:617: // Sort the
outputunits in descending order of the number of imports they
On 2014/04/02 09:32:48, floitsch wrote:
> output units

Done.

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:618: // include.
On 2014/04/02 09:32:48, floitsch wrote:
> Explain why this is necessary and enough.

I tried to explain it - hope it is not too verbose.

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3223: return new Future(() => null);
On 2014/04/02 09:32:48, floitsch wrote:
> new Future.value(null);

Done.

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3228: index < JS('int',
'\$.libraries_to_load[#].length', loadId);
On 2014/04/02 09:32:48, floitsch wrote:
> Move the JS outside the loop. This way we can have a one-line for.

Done.

https://codereview.chromium.org/221663005/diff/20001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3241: hunkNames.map((hunkName) =>
_loadHunk(hunkName, uri));
On 2014/04/02 09:32:48, floitsch wrote:
> indentation.

Done.

[floitsch, 2014-04-03 09:05:19]

Almost LGTM.

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

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:643: // Assuming
class C in output unit shared by imports in the set S1 =
Add: "By contradition:"

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3236: if (JS('bool',
'\$.libraries_to_load[#] === undefined', loadId)) {
libraries_to_load[#] is also a list. right?

List<List<String>> hunkLists = JS('JSExtendableArray|Null',
'\$.libraries_to_load[#], loadId);
if (hunkLists == null) return new Future.value(null);

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3240: new JSArray.markFixed(JS('=Object',
'\$.libraries_to_load[#]', loadId));
Why do you want to mark it as fixed?

[floitsch, 2014-04-03 09:05:42]

Please look at the output to verify that we generate compact JS.

On 2014/04/03 09:05:19, floitsch wrote:
> Almost LGTM.
> 
>
https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/compil...
> File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):
> 
>
https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/compil...
> sdk/lib/_internal/compiler/implementation/deferred_load.dart:643: // Assuming
> class C in output unit shared by imports in the set S1 =
> Add: "By contradition:"
> 
>
https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
> File sdk/lib/_internal/lib/js_helper.dart (right):
> 
>
https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
> sdk/lib/_internal/lib/js_helper.dart:3236: if (JS('bool',
> '\$.libraries_to_load[#] === undefined', loadId)) {
> libraries_to_load[#] is also a list. right?
> 
> List<List<String>> hunkLists = JS('JSExtendableArray|Null',
> '\$.libraries_to_load[#], loadId);
> if (hunkLists == null) return new Future.value(null);
> 
>
https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
> sdk/lib/_internal/lib/js_helper.dart:3240: new JSArray.markFixed(JS('=Object',
> '\$.libraries_to_load[#]', loadId));
> Why do you want to mark it as fixed?

[sigurdm, 2014-04-03 09:44:50]

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

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:643: // Assuming
class C in output unit shared by imports in the set S1 =
On 2014/04/03 09:05:20, floitsch wrote:
> Add: "By contradition:"

Done.

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3236: if (JS('bool',
'\$.libraries_to_load[#] === undefined', loadId)) {
On 2014/04/03 09:05:20, floitsch wrote:
> libraries_to_load[#] is also a list. right?
> 
> List<List<String>> hunkLists = JS('JSExtendableArray|Null',
> '\$.libraries_to_load[#], loadId);
> if (hunkLists == null) return new Future.value(null);

This works great

https://codereview.chromium.org/221663005/diff/80001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:3240: new JSArray.markFixed(JS('=Object',
'\$.libraries_to_load[#]', loadId));
On 2014/04/03 09:05:20, floitsch wrote:
> Why do you want to mark it as fixed?

Done.

[sigurdm, 2014-04-03 12:15:48]

Committed patchset #5 manually as r34698 (presubmit successful).

[floitsch, 2014-04-04 11:12:42]

Please make it clearer what changed after the last revert.

If it's just the change in the patch-file: LGTM.

[sigurdm, 2014-04-10 08:48:35]

Committed patchset #6 manually as r34915 (presubmit successful).