Use the entity model for dump_info.

Use the entity model for dump_info.

This changes dump_info to use the entity model and the visit methods in
ElementEnvironment. This way, once the Kernel implementation of
ElementEnvironment is fully implemented, dump_info should *just work*
with the common front end.

R=johnniwinther@google.com, sigmund@google.com

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

[Harry Terkelsen, 2017-08-29 18:29:57]

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

[Johnni Winther, 2017-08-30 13:52:35]

lgtm

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
pkg/compiler/lib/src/dump_info.dart:59:
compiler.libraryLoader.libraries.forEach(visitLibrary);
Change this to

  elementEnvironment.libraries.forEach(visitLibrary);

otherwise we'll mix K and J libraries.

[Siggi Cherem (dart-lang), 2017-08-30 17:30:43]

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

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common_elements.dart:1261: {bool ensureResolved: true});
since we don't really have "resolution" in the new pipeline, it seems odd to
expose the notion of "resolved" to the environment. I wonder if we should do
something different for this and `hasBeenResolved`.

For `hasBeenResolved` I have some ideas:

 - we could use `enqueuer.processedEntities.contains(member)` and
`enqueuer.processedClasses.contains(cls)`, but we need to make sure to use the
correct enqueuer: in the new world we have K and J elements, so the "resolution"
enqueuer will refer to the K elements, while the codegen refers to the J
elements. I believe you will be using the latter.

 - we could also use world.processedMembers (but may need to also add
processedClasses there)

For this function, I'm not sure what else to do, though.

Questions for Johnni: 
 - should we add processedClasses to world?
 - any preference between the alternatives above?
 - other ideas on what to do here?

I'm also OK landing with none or some of these changes, in which case let's just
add a "TODO(redemption)" to revisit these pieces later .

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
File pkg/compiler/lib/src/dump_info.dart (left):

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
pkg/compiler/lib/src/dump_info.dart:69: // TODO(sigmund): change the visit order
to eliminate the need to check
I wonder if we still need this.

I recall we run into some circularity in the past. I can't tell for sure from
your changes whether this is no longer possible.

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
pkg/compiler/lib/src/dump_info.dart:180: // TODO(het): add closureInfo size to
the size
delete? seems like you are already doing so?

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/codegen_world_builder.dart (right):

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/codegen_world_builder.dart:678: throw new
UnimplementedError(
Johnni - how do you suggest we implement this. Do we want the _elementMap to
have access to the KernelToLocalsMap? Or should we undo some of the abstractions
here and let dump-info directly find the corresponding local?

[Harry Terkelsen, 2017-08-30 21:28:50]

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

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common_elements.dart:1261: {bool ensureResolved: true});
On 2017/08/30 17:30:42, Siggi Cherem (dart-lang) wrote:
> since we don't really have "resolution" in the new pipeline, it seems odd to
> expose the notion of "resolved" to the environment. I wonder if we should do
> something different for this and `hasBeenResolved`.
> 
> For `hasBeenResolved` I have some ideas:
> 
>  - we could use `enqueuer.processedEntities.contains(member)` and
> `enqueuer.processedClasses.contains(cls)`, but we need to make sure to use the
> correct enqueuer: in the new world we have K and J elements, so the
"resolution"
> enqueuer will refer to the K elements, while the codegen refers to the J
> elements. I believe you will be using the latter.
> 
>  - we could also use world.processedMembers (but may need to also add
> processedClasses there)
> 
> For this function, I'm not sure what else to do, though.
> 
> Questions for Johnni: 
>  - should we add processedClasses to world?
>  - any preference between the alternatives above?
>  - other ideas on what to do here?
> 
> I'm also OK landing with none or some of these changes, in which case let's
just
> add a "TODO(redemption)" to revisit these pieces later .

Done, except for classes I'm still asking if it was resolved (through
MirrorsData) since I need to get the static members of classes that may never
have been instantiated.

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
File pkg/compiler/lib/src/dump_info.dart (left):

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
pkg/compiler/lib/src/dump_info.dart:69: // TODO(sigmund): change the visit order
to eliminate the need to check
On 2017/08/30 17:30:42, Siggi Cherem (dart-lang) wrote:
> I wonder if we still need this.
> 
> I recall we run into some circularity in the past. I can't tell for sure from
> your changes whether this is no longer possible.

It shouldn't be possible since we are always visiting sub-entities...

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
pkg/compiler/lib/src/dump_info.dart:59:
compiler.libraryLoader.libraries.forEach(visitLibrary);
On 2017/08/30 13:52:35, Johnni Winther wrote:
> Change this to
> 
>   elementEnvironment.libraries.forEach(visitLibrary);
> 
> otherwise we'll mix K and J libraries.

Done.

https://codereview.chromium.org/3008793002/diff/1/pkg/compiler/lib/src/dump_i...
pkg/compiler/lib/src/dump_info.dart:180: // TODO(het): add closureInfo size to
the size
On 2017/08/30 17:30:42, Siggi Cherem (dart-lang) wrote:
> delete? seems like you are already doing so?

Done.

[Siggi Cherem (dart-lang), 2017-08-30 21:51:49]

lgtm

I'm sure Johnni has a better suggestion for what to do about resolution below.
I'm OK pushing this forward though if you prefer.

https://codereview.chromium.org/3008793002/diff/20001/pkg/compiler/lib/src/du...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/3008793002/diff/20001/pkg/compiler/lib/src/du...
pkg/compiler/lib/src/dump_info.dart:379: return
compiler.enqueuer.resolution.processedEntities.contains(entity);
I believe using the `resolution` processedEntities might not be quite right
here.

I think it works with the element model, but in the new world it will contain
`K` elements, but we need to check for `J` elements.

I'm fine adding here a:
 // TODO(redemption)

Or in addition add:
 if (entity.library is JLibrary) return true;

(which probably equivalent to the "=> true" you had before)

[Harry Terkelsen, 2017-08-30 22:14:46]

https://codereview.chromium.org/3008793002/diff/20001/pkg/compiler/lib/src/du...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/3008793002/diff/20001/pkg/compiler/lib/src/du...
pkg/compiler/lib/src/dump_info.dart:379: return
compiler.enqueuer.resolution.processedEntities.contains(entity);
On 2017/08/30 21:51:48, Siggi Cherem (dart-lang) wrote:
> I believe using the `resolution` processedEntities might not be quite right
> here.
> 
> I think it works with the element model, but in the new world it will contain
> `K` elements, but we need to check for `J` elements.
> 
> I'm fine adding here a:
>  // TODO(redemption)
> 
> Or in addition add:
>  if (entity.library is JLibrary) return true;
> 
> (which probably equivalent to the "=> true" you had before)

Done.

[Harry Terkelsen, 2017-08-30 22:26:16]

Description was changed from

==========
Use the entity model for dump_info.

This changes dump_info to use the entity model and the visit methods in
ElementEnvironment. This way, once the Kernel implementation of
ElementEnvironment is fully implemented, dump_info should *just work*
with the common front end.
==========

to

==========
Use the entity model for dump_info.

This changes dump_info to use the entity model and the visit methods in
ElementEnvironment. This way, once the Kernel implementation of
ElementEnvironment is fully implemented, dump_info should *just work*
with the common front end.

R=johnniwinther@google.com, sigmund@google.com

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

[Harry Terkelsen, 2017-08-30 22:26:17]

Committed patchset #3 (id:40001) manually as
b3ce4a88e651c5bf4bfe67d0a8d6eec2b3b0e0a8 (presubmit successful).