dart2js: lazily load fields in their own deferred parts

dart2js: lazily load fields in their own deferred parts

Fixes #24356

BUG=
R=floitsch@google.com

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

[Harry Terkelsen, 2016-01-08 21:14:45]

het@google.com changed reviewers:
+ sigmund@google.com, sra@chromium.org

[Siggi Cherem (dart-lang), 2016-01-08 22:50:37]

nice!

I have more questions than comments :)

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (left):

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:717: // The code is
null if we ended up not needing the lazily
just to make sure I follow - we now emit null code also because we can't
distinguish between the code being inlined and code added later by a lazy
fragment?

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (right):

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:729:
laziesInfo.add(js.quoteName(namer.deriveLazyInitializerName(field.name)));
so this no longer makes the name globally unique, correct?

Does that mean that we need to make changes in other places to use the correct
calling convention? For example, do we need to update
isolateLazyInitializerAccess?

I can see that the startup-emitter does something similar to what you do here,
is that working properly or there might be a bug there too?

[Harry Terkelsen, 2016-01-11 18:01:21]

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (left):

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:717: // The code is
null if we ended up not needing the lazily
On 2016/01/08 22:50:37, Siggi Cherem (dart-lang) wrote:
> just to make sure I follow - we now emit null code also because we can't
> distinguish between the code being inlined and code added later by a lazy
> fragment?

We don't emit null code. The code here that filters the initializers is no
longer necessary because fragment.lazilyInitializedStaticFields has already
filtered out null code. It was necessary before because
handler.getLazilyInitializedFieldsForEmission() is not filtered.

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (right):

https://codereview.chromium.org/1575603002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:729:
laziesInfo.add(js.quoteName(namer.deriveLazyInitializerName(field.name)));
On 2016/01/08 22:50:37, Siggi Cherem (dart-lang) wrote:
> so this no longer makes the name globally unique, correct?
> 
> Does that mean that we need to make changes in other places to use the correct
> calling convention? For example, do we need to update
> isolateLazyInitializerAccess?
> 
> I can see that the startup-emitter does something similar to what you do here,
> is that working properly or there might be a bug there too?

It seems to be working properly in my simple test case for both the full emitter
and the startup emitter. It looks like the name is globally unique (a lazy
initializer named x in the main part is called 'x' and a lazy initializer named
x in a deferred part is named x0).

[Harry Terkelsen, 2016-01-11 21:25:33]

PTAL. I've fixed a 'type'-o

[Harry Terkelsen, 2016-01-11 21:33:51]

het@google.com changed reviewers:
+ floitsch@google.com

[Harry Terkelsen, 2016-01-11 21:33:52]

Florian, the code for lazy initializers in the full emitter now copies the code
you wrote to do the same thing in the startup emitter. We were wondering if
there were any corner cases we should be aware of since it seems likely that
there are many tricky example programs that could cause a polluted global
namespace.

Thanks!

[floitsch, 2016-01-12 13:38:38]

LGTM.
The code (full emitter) is not obvious, so I might have missed something, but
the approach looks good to me.

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (right):

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:699: if
(#isMainFragment) {
Do we do the right thing for "if / else" when the condition is a boolean?
Maybe you need to change it to two "ifs".

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:706: if (#notMinified)
{
ditto.

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:727: for (StaticField
field in lazies) {
Why don't you sort by position anymore?

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:730: if
(!compiler.enableMinification) {
maybe move the optional minification next to the optional holder.

(Not necessary, but it looks like you only need to change line 688+ and the line
here).

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:735:
laziesInfo.add(js('#', field.holder.name));
I believe that the field.holder.name will always be "$" (the static-state
holder), but that's probably ok.

The full emitter is a bit more complicated because it contains code for
incremental updates...

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:1583:
"lazyStaticFields": buildLazilyInitializedStaticFields(mainFragment),
I'm ok with this, but I prefer, when we only send in the data that is needed.
It makes it more explicit what buildLazilyInitializedStaticFields depends on.

[Harry Terkelsen, 2016-01-12 18:37:52]

Thanks, Florian

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (right):

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:699: if
(#isMainFragment) {
On 2016/01/12 13:38:38, floitsch wrote:
> Do we do the right thing for "if / else" when the condition is a boolean?
> Maybe you need to change it to two "ifs".

From examining the output, it looks like it is doing the right thing.

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:706: if (#notMinified)
{
On 2016/01/12 13:38:38, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:727: for (StaticField
field in lazies) {
On 2016/01/12 13:38:38, floitsch wrote:
> Why don't you sort by position anymore?

fragment.staticLazilyInitializedFields is already sorted by position. See
https://github.com/dart-lang/sdk/blob/d0f456a910675bab3b47b80fddf1fda5ee9b7fa...

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:730: if
(!compiler.enableMinification) {
On 2016/01/12 13:38:38, floitsch wrote:
> maybe move the optional minification next to the optional holder.
> 
> (Not necessary, but it looks like you only need to change line 688+ and the
line
> here).

Done.

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:735:
laziesInfo.add(js('#', field.holder.name));
On 2016/01/12 13:38:38, floitsch wrote:
> I believe that the field.holder.name will always be "$" (the static-state
> holder), but that's probably ok.
> 
> The full emitter is a bit more complicated because it contains code for
> incremental updates...

I think you're right. I considered doing 'namer.staticStateHolder' instead but
this seemed cleaner.

https://codereview.chromium.org/1575603002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:1583:
"lazyStaticFields": buildLazilyInitializedStaticFields(mainFragment),
On 2016/01/12 13:38:38, floitsch wrote:
> I'm ok with this, but I prefer, when we only send in the data that is needed.
> It makes it more explicit what buildLazilyInitializedStaticFields depends on.

Done.

[Harry Terkelsen, 2016-01-12 18:40:44]

Description was changed from

==========
dart2js: lazily load fields in their own deferred parts

Fixes #24356

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

to

==========
dart2js: lazily load fields in their own deferred parts

Fixes #24356

BUG=
R=floitsch@google.com

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

[Harry Terkelsen, 2016-01-12 18:40:45]

Committed patchset #5 (id:80001) manually as
c08bcf530d9664123540c75b3ceabf459260013d (presubmit successful).