Support deserialized compilation of the empty program.

Support deserialized compilation of the empty program.

R=sigmund@google.com

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

[Johnni Winther, 2016-04-15 15:11:59]

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

[Siggi Cherem (dart-lang), 2016-04-15 15:42:14]

lgtm

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/common...
File pkg/compiler/lib/src/common/resolution.dart (right):

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common/resolution.dart:186: abstract class Frontend {
I ponder about the name here.... "Frontend" makes sense in the context of the
backend, but it might be too broad when all of
parsing/resolution/deserialization is part of a frontend. 

Here are some other ideas:

- "Resolver" (although it feels incorrect, since it might not be the one doing
the resolution when it loads the data from a serialized format).

- "ResolutionProvider"?

If you are not convinced about the latter, we can keep Frontend while we
brainstorm for other ideas.

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common/resolution.dart:195: abstract class Resolution
extends Frontend {
it doesn't' really matter, but for some reason this feels like it should be
`implements` rather than `extends`?

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:550: _frontend = new
JSFrontendAccess(compiler);
seems it should be possible to make this an initializer and convert _frontend
into a `final frontend` instead

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:7886: void
enterInlinedMethod(FunctionElement function, ResolvedAst functionResolvedAst,
dartfmt

[Johnni Winther, 2016-04-18 08:05:43]

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/common...
File pkg/compiler/lib/src/common/resolution.dart (right):

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common/resolution.dart:186: abstract class Frontend {
On 2016/04/15 15:42:14, Siggi Cherem (dart-lang) wrote:
> I ponder about the name here.... "Frontend" makes sense in the context of the
> backend, but it might be too broad when all of
> parsing/resolution/deserialization is part of a frontend. 
> 
> Here are some other ideas:
> 
> - "Resolver" (although it feels incorrect, since it might not be the one doing
> the resolution when it loads the data from a serialized format).
> 
> - "ResolutionProvider"?
> 
> If you are not convinced about the latter, we can keep Frontend while we
> brainstorm for other ideas.
> 

'ResolutionProvider' is semantically the best but I don't like the 'Provider'
suffix. I don't particularly like 'Frontend' though it is fun to write
'compiler.backend.frontend' ;-)  Let's brainstorm a bit (added a TODO).

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common/resolution.dart:195: abstract class Resolution
extends Frontend {
On 2016/04/15 15:42:14, Siggi Cherem (dart-lang) wrote:
> it doesn't' really matter, but for some reason this feels like it should be
> `implements` rather than `extends`?

Done.

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:550: _frontend = new
JSFrontendAccess(compiler);
On 2016/04/15 15:42:14, Siggi Cherem (dart-lang) wrote:
> seems it should be possible to make this an initializer and convert _frontend
> into a `final frontend` instead

Done.

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/ssa/bu...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1892093003/diff/1/pkg/compiler/lib/src/ssa/bu...
pkg/compiler/lib/src/ssa/builder.dart:7886: void
enterInlinedMethod(FunctionElement function, ResolvedAst functionResolvedAst,
On 2016/04/15 15:42:14, Siggi Cherem (dart-lang) wrote:
> dartfmt

Done.

[Johnni Winther, 2016-04-18 08:36:42]

Description was changed from

==========
Support deserialized compilation of the empty program.
==========

to

==========
Support deserialized compilation of the empty program.

R=sigmund@google.com

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

[Johnni Winther, 2016-04-18 08:36:44]

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

[sra1, 2016-04-19 03:07:29]

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

[sra1, 2016-04-19 03:07:30]

Do you know why this slowed down compilation by 3%?

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:383: String toString() {
I prefer toString() methods to print so that they are not confusing if part of
another toString() output.  This usually means making a textual term with the
subparts in parens:

  ClosureScope(box=..., ...)

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:391: }
I usually do this like so:

  var separator = '';

  sb.write(separator);
  separator = ', ';
  sb.write(item);

It is the same number of lines, but you don't need to think about the predicate
and decouples separators from the other state.

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:399: capturedVariables.forEach((Local local,
BoxFieldElement field) {
Might as well just print the map.

[Johnni Winther, 2016-04-19 07:22:24]

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:383: String toString() {
On 2016/04/19 03:07:30, sra1 wrote:
> I prefer toString() methods to print so that they are not confusing if part of
> another toString() output.  This usually means making a textual term with the
> subparts in parens:
> 
>   ClosureScope(box=..., ...)
> 

Will do in a follow-up.

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:391: }
On 2016/04/19 03:07:30, sra1 wrote:
> I usually do this like so:
> 
>   var separator = '';
> 
>   sb.write(separator);
>   separator = ', ';
>   sb.write(item);
> 
> It is the same number of lines, but you don't need to think about the
predicate
> and decouples separators from the other state.

Will do in a follow-up.

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:399: capturedVariables.forEach((Local local,
BoxFieldElement field) {
On 2016/04/19 03:07:30, sra1 wrote:
> Might as well just print the map.

Some of the default toString implementations print only a subset of large
collections. Maybe that doesn't apply to Map?

[Johnni Winther, 2016-04-19 07:23:40]

On 2016/04/19 03:07:30, sra1 wrote:
> Do you know why this slowed down compilation by 3%?
> 
No. The only change in the main pipeline is the access of ResolvedAst.

[Siggi Cherem (dart-lang), 2016-04-19 20:58:02]

Thanks Stephen for catching that regression!

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1853: if
(compiler.serialization.isDeserialized(element)) {
Could this be a possible cause of the regression?  We are doing this check now
on every access to fetch the resolved AST.

I thought about the following during the review, then forgot to bring it up: I
wonder how early we can remove any distinction between deserialized & parsed
element models. Today we hide it behind this `resolution` abstraction, but
instead of doing it on queries, we might be able to do it a bit earlier. 
My hope is that by the time we load a serialized file, we initialize the pieces
of the compiler to avoid having to reach into the serialization task at all
afterwards.

For this specific use case, maybe we can remove the distinctions when we first
construct the class? For example if we implement the resolvedAst getter in
modelz and we store the deserialized impact in the _worldImpactCache, could we
use the same codepath here for both parsed & deserialized elements?

[Johnni Winther, 2016-04-20 10:40:21]

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1853: if
(compiler.serialization.isDeserialized(element)) {
On 2016/04/19 20:58:02, Siggi Cherem (dart-lang) wrote:
> Could this be a possible cause of the regression?  We are doing this check now
> on every access to fetch the resolved AST.
> 
> I thought about the following during the review, then forgot to bring it up: I
> wonder how early we can remove any distinction between deserialized & parsed
> element models. Today we hide it behind this `resolution` abstraction, but
> instead of doing it on queries, we might be able to do it a bit earlier. 
> My hope is that by the time we load a serialized file, we initialize the
pieces
> of the compiler to avoid having to reach into the serialization task at all
> afterwards.
> 
> For this specific use case, maybe we can remove the distinctions when we first
> construct the class? For example if we implement the resolvedAst getter in
> modelz and we store the deserialized impact in the _worldImpactCache, could we
> use the same codepath here for both parsed & deserialized elements?

For swarm SerializationTask.isDeserialized is called 73921 times, 27792 of these
from here. If find it surprising if that alone can create a 3% compile-time
increase.

[Siggi Cherem (dart-lang), 2016-04-20 18:58:40]

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1892093003/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1853: if
(compiler.serialization.isDeserialized(element)) {
On 2016/04/20 10:40:21, Johnni Winther wrote:
> On 2016/04/19 20:58:02, Siggi Cherem (dart-lang) wrote:
> > Could this be a possible cause of the regression?  We are doing this check
now
> > on every access to fetch the resolved AST.
> > 
> > I thought about the following during the review, then forgot to bring it up:
I
> > wonder how early we can remove any distinction between deserialized & parsed
> > element models. Today we hide it behind this `resolution` abstraction, but
> > instead of doing it on queries, we might be able to do it a bit earlier. 
> > My hope is that by the time we load a serialized file, we initialize the
> pieces
> > of the compiler to avoid having to reach into the serialization task at all
> > afterwards.
> > 
> > For this specific use case, maybe we can remove the distinctions when we
first
> > construct the class? For example if we implement the resolvedAst getter in
> > modelz and we store the deserialized impact in the _worldImpactCache, could
we
> > use the same codepath here for both parsed & deserialized elements?
> 
> For swarm SerializationTask.isDeserialized is called 73921 times, 27792 of
these
> from here. If find it surprising if that alone can create a 3% compile-time
> increase.

Yeah - I'm not sure, but this and the other changes to resolveAst could have
other effects for the VM, like making hasResolvedAst polymorphic (since now
there is a second implementation in the serialization task) or it can make
inlining of this method harder for the vm.

Just commenting the lines on this method doesn't justify a 3% regression (did
some perf testing that it only accounts for 0.3%), I'll look and see if
precomputing the `body` of a resolved AST is part of that cost.