Split World usage into open, inference, and closed world.

Split World usage into open, inference, and closed world.

This is a step towards separating the backend element model from the frontend model.

Main changes in world.dart.

R=het@google.com

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

[Johnni Winther, 2016-09-06 09:40:17]

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

[Siggi Cherem (dart-lang), 2016-09-06 20:23:09]

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/compil...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:307: World get openWorld => _world;
IMO - we reached the point were we need to rename universe and unify the
concepts we have. I honestly thought that "open world" could have been an
alternative name for "universe". I also thought we could call this
"resolutionWorld", but then I found we already have "resolverWorld" below, which
is a Universe... well, I admit, I'm confused :-)

I have a proposal (see below), but we might need to chat about the best course
of action (it might be better to do that change first and then see what subset
of the changes here you still need to include)? 

Here the proposal:
  - rename Universe to WorldBuilder

  - make World objects immutable (once they are built, they don't change
anymore)

  - introduce some notion of a WorldRefiner: something that takes a world and
makes a new world that is more precise (global inference would do this).**

  - make all worlds share the same API, and make all code that consumes the
world (e.g. ssa) agnostic about which world they are receiving. By default, some
APIs would be completely conservative (e.g. the world before inference would say
that all functions may be called in a loop).

  - internally the compiler will compute multiple worlds for the different
stages of compilation. But this is not exposed to any other task in the
compiler.



Altogether it would look something like this:

  - compiler creates a WorldBuilder for resolution (e.g. resolutionWorldBuilder)

  - Code under resolution only uses the WorldBuilder (there is no world yet).

  - compiler creates the first world:

      world = resolutionWorldBuilder.build(); // this replaces world.populate()

  - compiler runs global type-inference and gives it "world" as an input

  - after global type-inference inference runs, we create a new refined world:

    world = GlobalInferenceTask.refineWorld(world);

  - codegen is now doing a second tree-growing algorithm, so the compiler
compiler creates the `codegenWorldBuilder`.

  - finally we would build this super-refined world for any of the late linking
tasks we have in the compiler:

     world = codegenWorldBuilder.build(withEffectDataFrom: world);

Thoughts?


** technically these "refiners" are a way of making a better world, so maybe we
should call it a WorldAltruist ;-)... just kidding...

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:41: CommonMasks get commonMasks;
I really wonder whether we want to also expose these here or if we should pass
them explicitly where they are used.

It does feels like a worldly worthy thing, but I don't want "world" to become
the next "compiler" :)

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:176: abstract class ClosedWorld extends
ClassWorld {
+ dartdoc

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:220: abstract class InferenceWorld {
Interesting... this is good to see, because it highlights to me how the
information is flowing out of the global type-inference into the closed world. 

I'm not sure whether this belong here, though. Some methods below feel like a
way to provide input to the global type-inference task, so we should split that
out. Some methods are reporting the final result of type-inference, so maybe we
should expose first the result of type-inference and separately compute the
world that has those results?

This might become more clear once we resolve the longer redesign question I
raised earlier and as we make progress refactoring the global type-inference
task.

[Johnni Winther, 2016-09-19 13:03:49]

PTAL

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:41: CommonMasks get commonMasks;
On 2016/09/06 20:23:08, Siggi Cherem (dart-lang) wrote:
> I really wonder whether we want to also expose these here or if we should pass
> them explicitly where they are used.
> 
> It does feels like a worldly worthy thing, but I don't want "world" to become
> the next "compiler" :)

Removed. It was only need in an assertion where a workaround could be applied.

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:176: abstract class ClosedWorld extends
ClassWorld {
On 2016/09/06 20:23:09, Siggi Cherem (dart-lang) wrote:
> + dartdoc

Done.

https://codereview.chromium.org/2314703002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:220: abstract class InferenceWorld {
On 2016/09/06 20:23:08, Siggi Cherem (dart-lang) wrote:
> Interesting... this is good to see, because it highlights to me how the
> information is flowing out of the global type-inference into the closed world.

> 
> I'm not sure whether this belong here, though. Some methods below feel like a
> way to provide input to the global type-inference task, so we should split
that
> out. Some methods are reporting the final result of type-inference, so maybe
we
> should expose first the result of type-inference and separately compute the
> world that has those results?
> 
> This might become more clear once we resolve the longer redesign question I
> raised earlier and as we make progress refactoring the global type-inference
> task.

Renamed the class to [ClosedWorldRefiner] and add the `CurrentlyKnown` infix to
the getter to show that these current and not final results of the computation.

[Harry Terkelsen, 2016-09-19 17:18:25]

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

[Harry Terkelsen, 2016-09-19 17:18:27]

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

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:313: ClosedWorld get closedWorld => _world;
should we add an assert here that we can only access the closed world once we
know the world is actually closed?

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
File pkg/compiler/lib/src/enqueue.dart (left):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:387: compiler.world.allTypedefs.add(element);
I don't see where in this CL we register typedefs now that this is removed?

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:732:
"${compiler.inferenceWorld.getCurrentlyKnownMightBePassedToApply(e)}");
long line

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/enqueuer.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/enqueuer.dart:184: if
(universe.hasInvokedSetter(member, _world)) {
nit: maybe this if-statement can be merged with the previous one?

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:195: /// receiver of type [mask]. If multiple
targets exists, `null` is returned.
exists -> exist

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:199: /// receiver of type [mask]. If multiple
targets exists or the single target
exists -> exist

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:223: /// Interface for computing side effects
and used of elements. This is used
used -> uses

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:233: SideEffects
getCurrentlyKnownSideEffectsOfElement(Element element);
I think renaming this to 'getCurrentlyKnownSideEffects' would be more consistent
with the other names, what do you think?

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/concrete_type_inference_test.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/concrete_type_inference_test.dart:75:
compiler.commonMasks.stringType.containsOnlyString(compiler.closedWorld));
long line

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/dictionary_types_test.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dictionary_types_test.dart:115:
Expect.isTrue(getType('aString').containsOnlyString(compiler.closedWorld));
long line

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/gvn_dynamic_field_get_test.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/gvn_dynamic_field_get_test.dart:39:
Expect.isFalse(compiler.closedWorld.hasAnyUserDefinedGetter(selector, null));
long line

[Harry Terkelsen, 2016-09-19 17:29:43]

On 2016/09/19 17:18:27, Harry Terkelsen wrote:
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/co...
> File pkg/compiler/lib/src/compiler.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/co...
> pkg/compiler/lib/src/compiler.dart:313: ClosedWorld get closedWorld => _world;
> should we add an assert here that we can only access the closed world once we
> know the world is actually closed?
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
> File pkg/compiler/lib/src/enqueue.dart (left):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
> pkg/compiler/lib/src/enqueue.dart:387:
compiler.world.allTypedefs.add(element);
> I don't see where in this CL we register typedefs now that this is removed?
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/in...
> File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/in...
> pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:732:
> "${compiler.inferenceWorld.getCurrentlyKnownMightBePassedToApply(e)}");
> long line
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/js...
> File pkg/compiler/lib/src/js_backend/enqueuer.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_backend/enqueuer.dart:184: if
> (universe.hasInvokedSetter(member, _world)) {
> nit: maybe this if-statement can be merged with the previous one?
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
> File pkg/compiler/lib/src/world.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
> pkg/compiler/lib/src/world.dart:195: /// receiver of type [mask]. If multiple
> targets exists, `null` is returned.
> exists -> exist
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
> pkg/compiler/lib/src/world.dart:199: /// receiver of type [mask]. If multiple
> targets exists or the single target
> exists -> exist
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
> pkg/compiler/lib/src/world.dart:223: /// Interface for computing side effects
> and used of elements. This is used
> used -> uses
> 
>
https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
> pkg/compiler/lib/src/world.dart:233: SideEffects
> getCurrentlyKnownSideEffectsOfElement(Element element);
> I think renaming this to 'getCurrentlyKnownSideEffects' would be more
consistent
> with the other names, what do you think?
> 
>
https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
> File tests/compiler/dart2js/concrete_type_inference_test.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
> tests/compiler/dart2js/concrete_type_inference_test.dart:75:
> compiler.commonMasks.stringType.containsOnlyString(compiler.closedWorld));
> long line
> 
>
https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
> File tests/compiler/dart2js/dictionary_types_test.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
> tests/compiler/dart2js/dictionary_types_test.dart:115:
> Expect.isTrue(getType('aString').containsOnlyString(compiler.closedWorld));
> long line
> 
>
https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
> File tests/compiler/dart2js/gvn_dynamic_field_get_test.dart (right):
> 
>
https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
> tests/compiler/dart2js/gvn_dynamic_field_get_test.dart:39:
> Expect.isFalse(compiler.closedWorld.hasAnyUserDefinedGetter(selector, null));
> long line

I also noticed that in the enqueuer some calls to 'ensureResolved' were removed.
Why is that?

[Johnni Winther, 2016-09-20 08:29:58]

PTAL

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

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:313: ClosedWorld get closedWorld => _world;
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> should we add an assert here that we can only access the closed world once we
> know the world is actually closed?

Much harder said than done! It is in https://codereview.chromium.org/2349163003/
(3 CLs later!)

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
File pkg/compiler/lib/src/enqueue.dart (left):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:387: compiler.world.allTypedefs.add(element);
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> I don't see where in this CL we register typedefs now that this is removed?

It is a consequence of `typedef.ensureResolved(resolution)` (see
ResolverTask.resolveTypedef)

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:732:
"${compiler.inferenceWorld.getCurrentlyKnownMightBePassedToApply(e)}");
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> long line

Bad dartfmt! Fixed.

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/enqueuer.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/enqueuer.dart:184: if
(universe.hasInvokedSetter(member, _world)) {
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> nit: maybe this if-statement can be merged with the previous one?

Done.

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:195: /// receiver of type [mask]. If multiple
targets exists, `null` is returned.
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> exists -> exist

Done.

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:199: /// receiver of type [mask]. If multiple
targets exists or the single target
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> exists -> exist

Done.

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:223: /// Interface for computing side effects
and used of elements. This is used
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> used -> uses

Done.

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/wo...
pkg/compiler/lib/src/world.dart:233: SideEffects
getCurrentlyKnownSideEffectsOfElement(Element element);
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> I think renaming this to 'getCurrentlyKnownSideEffects' would be more
consistent
> with the other names, what do you think?

Good idea! Done.

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/concrete_type_inference_test.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/concrete_type_inference_test.dart:75:
compiler.commonMasks.stringType.containsOnlyString(compiler.closedWorld));
On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> long line

Done.

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/dictionary_types_test.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dictionary_types_test.dart:115:
Expect.isTrue(getType('aString').containsOnlyString(compiler.closedWorld));
On 2016/09/19 17:18:27, Harry Terkelsen wrote:
> long line

Done.

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/gvn_dynamic_field_get_test.dart (right):

https://codereview.chromium.org/2314703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/gvn_dynamic_field_get_test.dart:39:
Expect.isFalse(compiler.closedWorld.hasAnyUserDefinedGetter(selector, null));
On 2016/09/19 17:18:27, Harry Terkelsen wrote:
> long line

Done.

[Harry Terkelsen, 2016-09-20 17:47:41]

lgtm!

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
File pkg/compiler/lib/src/enqueue.dart (left):

https://codereview.chromium.org/2314703002/diff/20001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:387: compiler.world.allTypedefs.add(element);
On 2016/09/20 08:29:58, Johnni Winther wrote:
> On 2016/09/19 17:18:26, Harry Terkelsen wrote:
> > I don't see where in this CL we register typedefs now that this is removed?
> 
> It is a consequence of `typedef.ensureResolved(resolution)` (see
> ResolverTask.resolveTypedef)

Acknowledged.

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:729: ;
nit: delete useless semicolon

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:733:
.getCurrentlyKnownMightBePassedToApply(e)}");
this is syntactically valid?

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart (right):

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart:251: if
(!renamedSelector.appliesUnnamed(member, compiler.closedWorld))
nit: surround with curlies?

[Johnni Winther, 2016-09-21 07:44:53]

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:729: ;
On 2016/09/20 17:47:41, Harry Terkelsen wrote:
> nit: delete useless semicolon

Done.

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:733:
.getCurrentlyKnownMightBePassedToApply(e)}");
On 2016/09/20 17:47:41, Harry Terkelsen wrote:
> this is syntactically valid?

Yes. Within ${...} you can put any expression with arbitrary whitespace!

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart (right):

https://codereview.chromium.org/2314703002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart:251: if
(!renamedSelector.appliesUnnamed(member, compiler.closedWorld))
On 2016/09/20 17:47:41, Harry Terkelsen wrote:
> nit: surround with curlies?

Done.

[Johnni Winther, 2016-09-21 08:54:43]

Description was changed from

==========
Split World usage into open, inference, and closed world.

This is a step towards separating the backend element model from the frontend
model.

Main changes in world.dart.
==========

to

==========
Split World usage into open, inference, and closed world.

This is a step towards separating the backend element model from the frontend
model.

Main changes in world.dart.

R=het@google.com

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

[Johnni Winther, 2016-09-21 08:54:44]

Committed patchset #4 (id:60001) manually as
614cde5b1c980601e5595a7a873823aeedf62bc6 (presubmit successful).