When registering deferred constant, use canonical output unit.

When registering deferred constant, use canonical output unit.

Before, we were just creating a new output unit, which would cause
constants to map to unnamed output units.

BUG=
R=sigmund@google.com

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

[Harry Terkelsen, 2016-09-06 23:56:35]

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

[Harry Terkelsen, 2016-09-07 00:10:52]

Description was changed from

==========
When registering deferred constant, use canonical output unit.

Before, we were just creating a new output unit, which would cause
constants to map to unnamed output units.

BUG=
==========

to

==========
When registering deferred constant, use canonical output unit.

Before, we were just creating a new output unit, which would cause
constants to map to unnamed output units.

BUG=
==========

[Harry Terkelsen, 2016-09-07 00:10:52]

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

[Siggi Cherem (dart-lang), 2016-09-07 18:10:44]

I'm a bit puzzled - how come this breaks dump-info and it doesn't break our
emitter?

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:231: OutputUnit representative =
allOutputUnits.lookup(outputUnit);
I see this logic replicated in a few places, maybe we should pull it out into a
helper.

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:690: // resolution with
[registerConstantDeferredUse].
would it make sense to store those somewhere else instead?

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:750: _constantToOutputUnit[constant] =
representative;
not sure if this would work, but another option would be to post-process the
_constantToOutputUnit map:

  for (var k = _constantToOutputUnit.keys.toList()) {
    var ouputUnit = _constasntToOutputUnit[constant];
    var rep = representativeOf(outputUnit);
    if (!identical(outputUnit, rep)) _constasntToOutputUnit[constant] = rep;
  }

[Siggi Cherem (dart-lang), 2016-09-07 18:14:09]

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:231: OutputUnit representative =
allOutputUnits.lookup(outputUnit);
On 2016/09/07 18:10:44, Siggi Cherem (dart-lang) wrote:
> I see this logic replicated in a few places, maybe we should pull it out into
a
> helper.

reading more into this, it feels like we are doing some kind of unification in
how we use the allOutputUnits and operator==. It might not matter too much in
terms of performance, but maybe it is worth replacing this with a union-find
datastructure?

[Harry Terkelsen, 2016-09-07 23:53:50]

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:231: OutputUnit representative =
allOutputUnits.lookup(outputUnit);
On 2016/09/07 18:10:44, Siggi Cherem (dart-lang) wrote:
> I see this logic replicated in a few places, maybe we should pull it out into
a
> helper.

Done.

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:231: OutputUnit representative =
allOutputUnits.lookup(outputUnit);
On 2016/09/07 18:14:09, Siggi Cherem (dart-lang) wrote:
> On 2016/09/07 18:10:44, Siggi Cherem (dart-lang) wrote:
> > I see this logic replicated in a few places, maybe we should pull it out
into
> a
> > helper.
> 
> reading more into this, it feels like we are doing some kind of unification in
> how we use the allOutputUnits and operator==. It might not matter too much in
> terms of performance, but maybe it is worth replacing this with a union-find
> datastructure?

added a todo

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:690: // resolution with
[registerConstantDeferredUse].
On 2016/09/07 18:10:44, Siggi Cherem (dart-lang) wrote:
> would it make sense to store those somewhere else instead?

I wanted to do that at first, but it would mean having to differentiate between
when 'registerConstantDeferredUse' is called during resolution and when it is
called during codegen

https://codereview.chromium.org/2315973002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:750: _constantToOutputUnit[constant] =
representative;
On 2016/09/07 18:10:44, Siggi Cherem (dart-lang) wrote:
> not sure if this would work, but another option would be to post-process the
> _constantToOutputUnit map:
> 
>   for (var k = _constantToOutputUnit.keys.toList()) {
>     var ouputUnit = _constasntToOutputUnit[constant];
>     var rep = representativeOf(outputUnit);
>     if (!identical(outputUnit, rep)) _constasntToOutputUnit[constant] = rep;
>   }

the problem is that the 'registerConstantDeferredUse' can be called both before
this code and after

[Harry Terkelsen, 2016-09-07 23:54:39]

Thanks Siggi! PTAL

[Siggi Cherem (dart-lang), 2016-09-07 23:57:50]

lgtm

https://codereview.chromium.org/2315973002/diff/20001/pkg/compiler/lib/src/de...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2315973002/diff/20001/pkg/compiler/lib/src/de...
pkg/compiler/lib/src/deferred_load.dart:742: OutputUnit representative =
_getCanonicalUnit(outputUnit);
now that the logic is pulled out, consider removing the intermediate variable :)
(same elsewhere)

[Harry Terkelsen, 2016-09-08 00:01:30]

https://codereview.chromium.org/2315973002/diff/20001/pkg/compiler/lib/src/de...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2315973002/diff/20001/pkg/compiler/lib/src/de...
pkg/compiler/lib/src/deferred_load.dart:742: OutputUnit representative =
_getCanonicalUnit(outputUnit);
On 2016/09/07 23:57:50, Siggi Cherem (dart-lang) wrote:
> now that the logic is pulled out, consider removing the intermediate variable
:)
> (same elsewhere)

Done.

[Harry Terkelsen, 2016-09-08 00:01:44]

Description was changed from

==========
When registering deferred constant, use canonical output unit.

Before, we were just creating a new output unit, which would cause
constants to map to unnamed output units.

BUG=
==========

to

==========
When registering deferred constant, use canonical output unit.

Before, we were just creating a new output unit, which would cause
constants to map to unnamed output units.

BUG=
R=sigmund@google.com

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

[Harry Terkelsen, 2016-09-08 00:01:45]

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