Add deferred loading to Kernel.

Add deferred loading to Kernel.


BUG=
R=sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/063e60ae2916ac7de2f655bc91f99dcda3da6718

[Emily Fortuna, 2017-02-14 02:24:35]

Patchset #2 (id:20001) has been deleted

[Emily Fortuna, 2017-02-14 02:27:48]

Patchset #2 (id:40001) has been deleted

[Emily Fortuna, 2017-02-14 02:28:27]

efortuna@google.com changed reviewers:
+ sra@google.com

[sra1, 2017-02-14 03:14:06]

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2256: var deferredLibrary = new
ir.DeferredImport(
I would expect there to be a single shared DeferredImport.
There is a list of them in the Library.
How to they get added there?

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2275: expression = new
ir.LoadLibrary(deferredLibrary);
This code is the same as above.
I'd put it in a separate function so that if we threw our LoadLibrary and
directly make a js_runtime call, it would be in one place.

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:604: // previsit function to hold
the prefix element.
How are we going to fix this (getting the shorter name)?
The deferredLoadTask is still needed to track the chunks.
Can we call it here, or add a new entry point?

[Emily Fortuna, 2017-02-14 20:56:28]

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2256: var deferredLibrary = new
ir.DeferredImport(
On 2017/02/14 03:14:06, sra1 wrote:
> I would expect there to be a single shared DeferredImport.
> There is a list of them in the Library.
> How to they get added there?

Changed to PutIfAbsent. I'm not convinced this is an improvement though. It adds
a whole bunch of checks.

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2275: expression = new
ir.LoadLibrary(deferredLibrary);
On 2017/02/14 03:14:06, sra1 wrote:
> This code is the same as above.
> I'd put it in a separate function so that if we threw our LoadLibrary and
> directly make a js_runtime call, it would be in one place.

done. See above.

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:604: // previsit function to hold
the prefix element.
On 2017/02/14 03:14:06, sra1 wrote:
> How are we going to fix this (getting the shorter name)?
> The deferredLoadTask is still needed to track the chunks.
> Can we call it here, or add a new entry point?

This is what I was talking about last week. We can do so, but it requires a
previsit function to store the prefix element until we reach this function. We
can also write a variant of the "make unique" URI function that we're doing in
the original version, but it seems like it ties us closer to the element model
with very little benefit for a lot of extra work.

[Emily Fortuna, 2017-02-14 21:32:47]

Patchset #3 (id:80001) has been deleted

[sra1, 2017-02-14 21:55:28]

lgtm

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:604: // previsit function to hold
the prefix element.
On 2017/02/14 20:56:28, Emily Fortuna wrote:
> On 2017/02/14 03:14:06, sra1 wrote:
> > How are we going to fix this (getting the shorter name)?
> > The deferredLoadTask is still needed to track the chunks.
> > Can we call it here, or add a new entry point?
> 
> This is what I was talking about last week. We can do so, but it requires a
> previsit function to store the prefix element until we reach this function. We
> can also write a variant of the "make unique" URI function that we're doing in
> the original version, but it seems like it ties us closer to the element model
> with very little benefit for a lot of extra work.

I'm concerned that two builds will generate different code because of generated
libraries being in some different tmp directory each build.
Or if you build and I build, we get our home directory aka user names in the
generated code.

State in the TODO that we want a shorter name that is independent of the
directory in which the source tree lives.

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:208: // "import foo.dart
deferred as bar").
Is the 'underlying' library element the current library or the imported library?

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2262: var prefixToDeferredImport
= deferredImports[importedLibraryElement];
Nowadays I find ??= more convenient

var map = deferredImports[importedLibraryElement] ??= <...>{};
return deferredImports[prefix.name] ??= new ...

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2271:
deferredImports[importedLibraryElement] = {prefix.name: deferredImport};
Type parameters on literal.

[Emily Fortuna, 2017-02-15 01:02:31]

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2691753002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:604: // previsit function to hold
the prefix element.
On 2017/02/14 21:55:27, sra1 wrote:
> On 2017/02/14 20:56:28, Emily Fortuna wrote:
> > On 2017/02/14 03:14:06, sra1 wrote:
> > > How are we going to fix this (getting the shorter name)?
> > > The deferredLoadTask is still needed to track the chunks.
> > > Can we call it here, or add a new entry point?
> > 
> > This is what I was talking about last week. We can do so, but it requires a
> > previsit function to store the prefix element until we reach this function.
We
> > can also write a variant of the "make unique" URI function that we're doing
in
> > the original version, but it seems like it ties us closer to the element
model
> > with very little benefit for a lot of extra work.
> 
> I'm concerned that two builds will generate different code because of
generated
> libraries being in some different tmp directory each build.
> Or if you build and I build, we get our home directory aka user names in the
> generated code.
> 
> State in the TODO that we want a shorter name that is independent of the
> directory in which the source tree lives.

Fixed it properly.

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:208: // "import foo.dart
deferred as bar").
On 2017/02/14 21:55:28, sra1 wrote:
> Is the 'underlying' library element the current library or the imported
library?

updated the comment -- imported library.

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2262: var prefixToDeferredImport
= deferredImports[importedLibraryElement];
On 2017/02/14 21:55:28, sra1 wrote:
> Nowadays I find ??= more convenient
> 
> var map = deferredImports[importedLibraryElement] ??= <...>{};
> return deferredImports[prefix.name] ??= new ...

Done.

https://codereview.chromium.org/2691753002/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:2271:
deferredImports[importedLibraryElement] = {prefix.name: deferredImport};
On 2017/02/14 21:55:28, sra1 wrote:
> Type parameters on literal.

Done.

[Emily Fortuna, 2017-02-15 01:51:57]

Description was changed from

==========
Add deferred loading to Kernel.


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

to

==========
Add deferred loading to Kernel.


BUG=
R=sra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/063e60ae2916ac7de2f655bc91f99dcda3da6718
==========

[Emily Fortuna, 2017-02-15 01:51:59]

Committed patchset #5 (id:140001) manually as
063e60ae2916ac7de2f655bc91f99dcda3da6718 (presubmit successful).