Build top-level variables and property accessors lazily.

Build top-level variables and property accessors lazily.

This makes DDC compilation of the latest big test codebase about 25%
faster - from 2900 ms to 2196 ms.

We also have to compute lazily public and export namespaces.

R=paulberry@google.com, brianwilkerson@google.com
BUG=

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

[Paul Berry, 2016-05-19 11:27:30]

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (left):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1232: 
Nit: restore this blank line.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1301:
implicitAccessor.enclosingElement = this;
If `.accessors` is called and then `.topLevelVariables` is called, this code
will get unnecessarily executed twice.  Same goes for
`implicitVariable.enclosingElement = this;` on line 1305.

How about if instead we pass `this` to
`resynthesizerContext.buildTopLevelAccessors()` and
`resynthesizerContext.buildTopLevelVariables()`, so that they can set the
enclosing element properly when they create the elements?

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1449: * TODO(scheglov) When
`_DeferredClassElement` is replaced with
In addition to the TODO, I would appreciate a longer documentation comment
explaining how this method differs from "getChild" and why one would want to
call one instead of the other (I had to do some digging before the reason was
clear to me).

Perhaps something like: "Equivalent to [getChild], but does not make the
assumption that the child implements [ElementImpl].  Intended as a temporary
measure while element model resynthesis is being made lazy."

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1530: _topLevelVariableReplaceMap
??=
This will only have the desired effect if the `topLevelVariables` getter hasn't
been called yet.  Please add a comment to this effect and the assertion:

assert(_variables == null);

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:4453: * TODO(scheglov) document
Did you intend to address this before sending out for review?

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/resynthesize.dart (right):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:136: Element element =
unitElement?.getChildNotImpl(components[2]);
I have an efficiency concern here.  You're replacing a map lookup with a linear
search.  Can we change the implementation of `getChildNotImpl` to use an
internal map?

(Note that since the intention is to eventually replace `getChildNotImpl` with
`getChild`, we should make a similar change to `getChild`).

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:1355: class
_LibraryResynthesizerContext implements LibraryResynthesizerContext {
Nit: a doc comment would be helpful here.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:2214: PropertyAccessorElementImpl
buildImplicitGetter(
Please add a doc comment here.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:2229: PropertyAccessorElementImpl
buildImplicitSetter(
Ditto.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/resynthesize_test.dart (right):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/resynthesize_test.dart:263:
expect(resynthesized.types.length, original.types.length, reason: 'types');
Please include `$desc` in the reason (e.g. `reason: '$desc types'`).  It makes
debugging failures so much easier.

[scheglov, 2016-05-19 16:54:57]

PTAL

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1301:
implicitAccessor.enclosingElement = this;
On 2016/05/19 11:27:30, Paul Berry wrote:
> If `.accessors` is called and then `.topLevelVariables` is called, this code
> will get unnecessarily executed twice.  Same goes for
> `implicitVariable.enclosingElement = this;` on line 1305.
> 
> How about if instead we pass `this` to
> `resynthesizerContext.buildTopLevelAccessors()` and
> `resynthesizerContext.buildTopLevelVariables()`, so that they can set the
> enclosing element properly when they create the elements?

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1449: * TODO(scheglov) When
`_DeferredClassElement` is replaced with
On 2016/05/19 11:27:30, Paul Berry wrote:
> In addition to the TODO, I would appreciate a longer documentation comment
> explaining how this method differs from "getChild" and why one would want to
> call one instead of the other (I had to do some digging before the reason was
> clear to me).
> 
> Perhaps something like: "Equivalent to [getChild], but does not make the
> assumption that the child implements [ElementImpl].  Intended as a temporary
> measure while element model resynthesis is being made lazy."

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1530: _topLevelVariableReplaceMap
??=
On 2016/05/19 11:27:29, Paul Berry wrote:
> This will only have the desired effect if the `topLevelVariables` getter
hasn't
> been called yet.  Please add a comment to this effect and the assertion:
> 
> assert(_variables == null);

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:4453: * TODO(scheglov) document
On 2016/05/19 11:27:29, Paul Berry wrote:
> Did you intend to address this before sending out for review?

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/resynthesize.dart (right):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:136: Element element =
unitElement?.getChildNotImpl(components[2]);
On 2016/05/19 11:27:30, Paul Berry wrote:
> I have an efficiency concern here.  You're replacing a map lookup with a
linear
> search.  Can we change the implementation of `getChildNotImpl` to use an
> internal map?
> 
> (Note that since the intention is to eventually replace `getChildNotImpl` with
> `getChild`, we should make a similar change to `getChild`).

That's fair.
I think that the approach with `getChildNotImpl` was not good enough.
So, I'm removing it and switch to building <name, Element>{} maps lazily.

Ideally however we want to avoid building this map at all and resynthesize only
the elements we are asked.
But this is a long tail - we need to make changes to namespaces.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:1355: class
_LibraryResynthesizerContext implements LibraryResynthesizerContext {
On 2016/05/19 11:27:30, Paul Berry wrote:
> Nit: a doc comment would be helpful here.

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:2214: PropertyAccessorElementImpl
buildImplicitGetter(
On 2016/05/19 11:27:30, Paul Berry wrote:
> Please add a doc comment here.

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:2229: PropertyAccessorElementImpl
buildImplicitSetter(
On 2016/05/19 11:27:30, Paul Berry wrote:
> Ditto.

Done.

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/resynthesize_test.dart (right):

https://codereview.chromium.org/1995763003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/resynthesize_test.dart:263:
expect(resynthesized.types.length, original.types.length, reason: 'types');
On 2016/05/19 11:27:30, Paul Berry wrote:
> Please include `$desc` in the reason (e.g. `reason: '$desc types'`).  It makes
> debugging failures so much easier.

Done.

[Paul Berry, 2016-05-19 17:10:32]

lgtm, thanks!

[scheglov, 2016-05-19 17:12:40]

Description was changed from

==========
Build top-level variables and property accessors lazily.

This makes DDC compilation of the latest big test codebase about 25%
faster - from 2900 ms to 2196 ms.

We also have to compute lazily public and export namespaces.

R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
Build top-level variables and property accessors lazily.

This makes DDC compilation of the latest big test codebase about 25%
faster - from 2900 ms to 2196 ms.

We also have to compute lazily public and export namespaces.

R=paulberry@google.com, brianwilkerson@google.com
BUG=

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

[scheglov, 2016-05-19 17:12:41]

Committed patchset #2 (id:20001) manually as
54e58c7a44db289d2803bebe3162fb3cf65e4add (presubmit successful).