fix #29182, generate top level const fields lazily

fix #29182, generate top level const fields lazily
This fixes an ordering issue, but also may help load time.

R=vsm@google.com

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

[Jennifer Messerly, 2017-04-04 22:38:50]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[vsm, 2017-04-04 22:58:43]

lgtm!

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (left):

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:4078: ClassElement
classElem, VariableDeclaration field) {
We could consider keeping this/below only for cases where the initializer is a
primitive literal ("foo", 42, null) - i.e., doesn't affect order.

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:539:
_topLevelElements.add(e);
Is it worth asserting that it's not there already?

[Jennifer Messerly, 2017-04-04 23:05:17]

Main question: should we land now and work on improvements as another issue
(either right after or prioritized differently)? or do you wanna see that change
before the bug fix lands?

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (left):

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:4078: ClassElement
classElem, VariableDeclaration field) {
On 2017/04/04 22:58:43, vsm wrote:
> We could consider keeping this/below only for cases where the initializer is a
> primitive literal ("foo", 42, null) - i.e., doesn't affect order.

yeah... thought about that. It'd certainly be a lot nicer looking. What are your
thougths about emitting it slightly out of order in a group? e.g.

final x = const Something();
final y = 42;
final z = x;

should we emit "y" after or before? it's nice if we group "x" and "z" into the
same "defineLazy" call.

BTW yet another option ... I dunno how fancy we wanna get, but this is another
option to consider for the mixes of lazy and non-lazy ...

defineLazy(theLib, {
  get x()  { return dart.const(new Something()); },
  y: 42,
  get z() { return theLib.x; }
});

[vsm, 2017-04-04 23:14:40]

On 2017/04/04 23:05:17, Jennifer Messerly wrote:
> Main question: should we land now and work on improvements as another issue
> (either right after or prioritized differently)? or do you wanna see that
change
> before the bug fix lands?

I would land first.  :-)

>
https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
> File pkg/dev_compiler/lib/src/compiler/code_generator.dart (left):
> 
>
https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
> pkg/dev_compiler/lib/src/compiler/code_generator.dart:4078: ClassElement
> classElem, VariableDeclaration field) {
> On 2017/04/04 22:58:43, vsm wrote:
> > We could consider keeping this/below only for cases where the initializer is
a
> > primitive literal ("foo", 42, null) - i.e., doesn't affect order.
> 
> yeah... thought about that. It'd certainly be a lot nicer looking. What are
your
> thougths about emitting it slightly out of order in a group? e.g.
> 
> final x = const Something();
> final y = 42;
> final z = x;
> 
> should we emit "y" after or before? it's nice if we group "x" and "z" into the
> same "defineLazy" call.
> 
> BTW yet another option ... I dunno how fancy we wanna get, but this is another
> option to consider for the mixes of lazy and non-lazy ...
> 
> defineLazy(theLib, {
>   get x()  { return dart.const(new Something()); },
>   y: 42,
>   get z() { return theLib.x; }
> });

That's pretty nice!  The mild reorderings above seem reasonable to me to though
- we're already doing some anyway.

[Jennifer Messerly, 2017-04-04 23:18:34]

Thanks! landing but will file a bug linking commit shortly

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2797443007/diff/10001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:539:
_topLevelElements.add(e);
On 2017/04/04 22:58:43, vsm wrote:
> Is it worth asserting that it's not there already?  

I think it's ~probably okay if we skip that assert. My worry is checking
.contains adds an O(N) check... could make checked mode a lot slower. All that
really matters is we know when we're in top level code and when we aren't.
Conceptually this is kinda part of the current element stack, it's as if we have
a stack like:

library foo
class C
class C - TOP LEVEL
class D
class D - TOP LEVEL
class E

[Jennifer Messerly, 2017-04-04 23:18:55]

Description was changed from

==========
fix #29182, generate top level const fields lazily
This fixes an ordering issue, but also may help load time.
==========

to

==========
fix #29182, generate top level const fields lazily
This fixes an ordering issue, but also may help load time.

R=vsm@google.com

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

[Jennifer Messerly, 2017-04-04 23:18:56]

Committed patchset #2 (id:10001) manually as
e4e7dffadb28975e834920662144bcfd6f9188b5 (presubmit successful).

[Jennifer Messerly, 2017-04-04 23:20:33]

https://github.com/dart-lang/sdk/issues/29253