Serialize more properties of ConstructorElement.

Serialize more properties of ConstructorElement.

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

[Johnni Winther, 2016-04-29 06:56:17]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Johnni Winther, 2016-04-29 07:59:54]

TBR

(needed to land https://codereview.chromium.org/1927963002/ but I forgot to
publish it yesterday)

[Johnni Winther, 2016-04-29 08:01:00]

Description was changed from

==========
Serialize more properties of ConstructorElement.
==========

to

==========
Serialize more properties of ConstructorElement.

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

[Johnni Winther, 2016-04-29 08:01:02]

Committed patchset #2 (id:20001) manually as
4c563fecbf0be73d2d1df5bd005bca415a9065eb (presubmit successful).

[Siggi Cherem (dart-lang), 2016-04-29 20:20:23]

lgtm

https://codereview.chromium.org/1924053003/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1924053003/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:1218: ConstructorElement get
immediateRedirectionTarget {
optional suggestion, probably not worth it.

I'd either initialize them everything we can upfront in the constructor. Any
lazy field, I'd prefer to initialize on its own if that's possible. For example
these 2 redirection targets are technically always used together, but they could
easily be initialized each on their first use. 

ConstructorElement get immediateRedirectionTarget =>
    __immediateRedirectionTarget ??=
decoder.getElement(Key.IMMEDIATE_REDIRECTION_TARGET);

ConstructorElement get redirectionDeferredPRefix =>
    __immediateRedirectionTarget ??= decoder.getElement(Key.PREFIX, ...);



I think we can do something similar with effectiveTarget/Type, but again, might
not be worth it.

[Johnni Winther, 2016-04-30 09:04:46]

https://codereview.chromium.org/1924053003/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1924053003/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:1218: ConstructorElement get
immediateRedirectionTarget {
On 2016/04/29 20:20:23, Siggi Cherem (dart-lang) wrote:
> optional suggestion, probably not worth it.
> 
> I'd either initialize them everything we can upfront in the constructor. Any
> lazy field, I'd prefer to initialize on its own if that's possible. 


I think it's only really worthwhile to deserialize everything in the constructor
if we can free the [_decoder] and more importantly the object from which it
reads its data. With the current setup it is not directly possible but we need
to look into it when we focus on the serialization format.

> For example
> these 2 redirection targets are technically always used together, but they
could
> easily be initialized each on their first use. 
> 
> ConstructorElement get immediateRedirectionTarget =>
>     __immediateRedirectionTarget ??=
> decoder.getElement(Key.IMMEDIATE_REDIRECTION_TARGET);
> 
> ConstructorElement get redirectionDeferredPRefix =>
>     __immediateRedirectionTarget ??= decoder.getElement(Key.PREFIX, ...);
> 

Since [_redirectionDeferredPrefix] might be `null` ??= would make us decode on
each access. This is why I grouped it with [immediateRedirectionTarget].

> 
> 
> I think we can do something similar with effectiveTarget/Type, but again,
might
> not be worth it.

[Siggi Cherem (dart-lang), 2016-05-02 16:58:35]

https://codereview.chromium.org/1924053003/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1924053003/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:1218: ConstructorElement get
immediateRedirectionTarget {
On 2016/04/30 09:04:46, Johnni Winther wrote:
> On 2016/04/29 20:20:23, Siggi Cherem (dart-lang) wrote:
> > optional suggestion, probably not worth it.
> > 
> > I'd either initialize them everything we can upfront in the constructor. Any
> > lazy field, I'd prefer to initialize on its own if that's possible. 
> 
> 
> I think it's only really worthwhile to deserialize everything in the
constructor
> if we can free the [_decoder] and more importantly the object from which it
> reads its data. With the current setup it is not directly possible but we need
> to look into it when we focus on the serialization format.
> 

Yeah, I look forward to the new format. At that point we'll likely initialize
everything upfront: looking at Asger's numbers for the time spent loading data,
I expect it will be fast enough that it wont be worth making it lazy.

> > For example
> > these 2 redirection targets are technically always used together, but they
> could
> > easily be initialized each on their first use. 
> > 
> > ConstructorElement get immediateRedirectionTarget =>
> >     __immediateRedirectionTarget ??=
> > decoder.getElement(Key.IMMEDIATE_REDIRECTION_TARGET);
> > 
> > ConstructorElement get redirectionDeferredPRefix =>
> >     __immediateRedirectionTarget ??= decoder.getElement(Key.PREFIX, ...);
> > 
> 
> Since [_redirectionDeferredPrefix] might be `null` ??= would make us decode on
> each access. This is why I grouped it with [immediateRedirectionTarget].
> 
> > 
> > 
> > I think we can do something similar with effectiveTarget/Type, but again,
> might
> > not be worth it.
> 
> 

Ah, I see. Maybe we should consider initializing in constructors anything we
that says `isOptional`.