Deferred (aka lazy) loading of static functions.

Deferred (aka lazy) loading of static functions.

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

[ahe, 2013-02-04 17:46:29]

Request for initial comments. Please take a look at:

dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart

I'm including everything for context, but it isn't ready. I plan to split this
into multiple CLs, for example, the parsing of metadata on libraries is a good
candidate for a standalone CL.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:526: // Used
to implement deferred loading. Used as callback on "load"
I should be able to move these methods into async_patch now that _foreign_helper
is imported explicitly.

[kasperl, 2013-02-05 08:30:53]

Looks like pretty good progress to me. The big missing piece is probably
coloring the import graph so that libraries only loaded from deferred loaded
libraries do not have to be eagerly loaded (or explicitly marked as deferred),
but that probably doesn't go into the first CL.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:8: final
Set<LibraryElement> deferredLibraries = new Set<LibraryElement>();
I guess this will have to be replaced with some sort of graph coloring algorithm
so libraries loaded from deferred loaded libraries also get deferred (if
possible).

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart
(right):

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:27: patch
Future<bool> load(String libraryName, {String uri}) {
How should this behave if the specified library name isn't deferred loaded?
Somehow it seems useful to be able to toggle the @DeferLoad annotation on and
off (for testing and measuring code size impact) so it would sort of make sense
to allow calling load with a library that is actually eagerly loaded.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:43: if
(JS('String', 'typeof document') == 'object') {
Use _hasDocument?

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:50: JS('',
'#.bind(null, #)', Primitives.onDeferredLibraryLoad(), completer);
4 space indent.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:53: } else
if (JS('String', 'typeof read') == 'function') {
Not sure I understand this. If the function 'read' is defined you call 'load'?

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:78: var
stack = JS('String|Null', 'throw new Error().stack');
Why doesn't this throw an uncaught exception? Did you mean to remove 'throw'
here?

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

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1066: //
TODO(ahe): What's the harm in inlining from a lazily loaded
Good question. I guess you still want the code to throw an exception if you
haven't loaded the library yet (the load future hasn't completed) which may be
more difficult if you inline. So I guess the issue with inlining these elements
is that it changes the semantics?

https://codereview.chromium.org/12033003/diff/2001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js_extra/deferred/deferred_function_test.dart
(right):

https://codereview.chromium.org/12033003/diff/2001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js_extra/deferred/deferred_function_test.dart:10:
@DeferLoad('deferred_function_library')
If we had support for it, would it make sense to make this @DeferLoad annotation
optional (multitest style)? I guess it is part of the design that the code
should work just fine if you ignore the annotation, right?

[ahe, 2013-02-05 08:33:08]

Thank you for taking a look, Kasper!

On 2013/02/05 08:30:53, kasperl wrote:
> Looks like pretty good progress to me. The big missing piece is probably
> coloring the import graph so that libraries only loaded from deferred loaded
> libraries do not have to be eagerly loaded (or explicitly marked as deferred),
> but that probably doesn't go into the first CL.

I'm not concerned about coloring. That graph is easy to build :-)

What worries me right now is dynamically loading a class.

Cheers,
Peter

[ahe, 2013-02-05 13:54:22]

Thank you for taking an early look. I have uploaded a new patch set which
addresses your comments (but it is still not ready).

I have been working on submitting other standalone changes, and I will rebase
this CL soon. It will be smaller then.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:8: final
Set<LibraryElement> deferredLibraries = new Set<LibraryElement>();
On 2013/02/05 08:30:53, kasperl wrote:
> I guess this will have to be replaced with some sort of graph coloring
algorithm
> so libraries loaded from deferred loaded libraries also get deferred (if
> possible).

Correct. Added a TODO.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart
(right):

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:43: if
(JS('String', 'typeof document') == 'object') {
On 2013/02/05 08:30:53, kasperl wrote:
> Use _hasDocument?

Done.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:50: JS('',
'#.bind(null, #)', Primitives.onDeferredLibraryLoad(), completer);
On 2013/02/05 08:30:53, kasperl wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:53: } else
if (JS('String', 'typeof read') == 'function') {
On 2013/02/05 08:30:53, kasperl wrote:
> Not sure I understand this. If the function 'read' is defined you call 'load'?

It's a mistake. I got the name wrong first :-)

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:78: var
stack = JS('String|Null', 'throw new Error().stack');
On 2013/02/05 08:30:53, kasperl wrote:
> Why doesn't this throw an uncaught exception? Did you mean to remove 'throw'
> here?

Yes. This was caught by my tests, but after having tested I changed this code to
support IE, and forgot to rerun test before uploading.

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:526: // Used
to implement deferred loading. Used as callback on "load"
On 2013/02/04 17:46:29, ahe wrote:
> I should be able to move these methods into async_patch now that
_foreign_helper
> is imported explicitly.

Done.

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

https://codereview.chromium.org/12033003/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1066: //
TODO(ahe): What's the harm in inlining from a lazily loaded
On 2013/02/05 08:30:53, kasperl wrote:
> Good question. I guess you still want the code to throw an exception if you
> haven't loaded the library yet (the load future hasn't completed) which may be
> more difficult if you inline. So I guess the issue with inlining these
elements
> is that it changes the semantics?

Good point.

https://codereview.chromium.org/12033003/diff/2001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js_extra/deferred/deferred_function_test.dart
(right):

https://codereview.chromium.org/12033003/diff/2001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js_extra/deferred/deferred_function_test.dart:10:
@DeferLoad('deferred_function_library')
On 2013/02/05 08:30:53, kasperl wrote:
> If we had support for it, would it make sense to make this @DeferLoad
annotation
> optional (multitest style)? I guess it is part of the design that the code
> should work just fine if you ignore the annotation, right?

Not sure about the "didLoad" parameter below. I think this particular test would
fail if the library isn't lazily loaded.

[ahe, 2013-02-05 14:04:03]

Uploaded patch set 3 which is rebased on top of changes I have already
submitted. The CL is smaller now, but still not ready. I'm just sending this
message so you know what patch set 3 is.

[sra1, 2013-02-12 01:44:47]

https://codereview.chromium.org/12033003/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12033003/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1068: //
TODO(ahe): But we shold still inline into the same
shold -> should

[ahe, 2013-02-18 20:19:01]

I'm ready to try to submit this. PTAL.

https://codereview.chromium.org/12033003/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12033003/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1068: //
TODO(ahe): But we shold still inline into the same
On 2013/02/12 01:44:47, sra1 wrote:
> shold -> should

Done.

[ngeoffray, 2013-02-19 10:04:57]

LGTM!

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

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:27: class
DeferredLoadTask extends CompilerTask {
Is there some time you would like to measure for this task?

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1785: //
TODO(ahe): Defer loading of these getters.
warnNotImplemented?

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart
(right):

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:66: int
index = JS('int', '#.lastIndexOf("/")', uri);
It says it's a URI, but did you check it worked on windows?

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:67: uri =
JS('String', '#.substring(0, # + 1) + #', uri, index, "part.js");
Why don't you use Dart String methods for substring and lastIndexOf?

[ahe, 2013-02-19 11:10:36]

PTAL at:

https://codereview.chromium.org/12033003/diff2/27002:27003/dart/sdk/lib/_inte...
https://codereview.chromium.org/12033003/diff2/27002:27003/dart/sdk/lib/_inte...
https://codereview.chromium.org/12033003/diff2/27002:27003/dart/sdk/lib/_inte...

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

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/deferred_load.dart:27: class
DeferredLoadTask extends CompilerTask {
On 2013/02/19 10:04:57, ngeoffray wrote:
> Is there some time you would like to measure for this task?

Done.

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1785: //
TODO(ahe): Defer loading of these getters.
On 2013/02/19 10:04:57, ngeoffray wrote:
> warnNotImplemented?

The overhead is small, and I can't give a location. So I thought it was better
to not warn. Remember the element is the function that needs a getter, I don't
know where that use is.

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart
(right):

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:66: int
index = JS('int', '#.lastIndexOf("/")', uri);
On 2013/02/19 10:04:57, ngeoffray wrote:
> It says it's a URI, but did you check it worked on windows?

Yes. It is a URL.

https://codereview.chromium.org/12033003/diff/35001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:67: uri =
JS('String', '#.substring(0, # + 1) + #', uri, index, "part.js");
On 2013/02/19 10:04:57, ngeoffray wrote:
> Why don't you use Dart String methods for substring and lastIndexOf?

I was trying to avoid bringing unnecessary "junk" into the system. But since I
use Set, Completer, and Future, we're hosed anyways.

[ngeoffray, 2013-02-19 11:34:13]

LGTM