Test closed world model after deserialization.

Test closed world model after deserialization.

We still need better tracking of native/foreign elements.

R=sigmund@google.com

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

[Johnni Winther, 2016-04-05 14:34:04]

Description was changed from

==========
Initial testing of closed world model after deserialization.

Currently we cannot assert much, mostly due to native/foreign elements and the
tracking of these.
==========

to

==========
Test closed world model after deserialization.

We still need better tracking of native/foreign elements.
==========

[Johnni Winther, 2016-04-05 14:34:16]

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

[Siggi Cherem (dart-lang), 2016-04-05 15:19:30]

lgtm

https://codereview.chromium.org/1856953003/diff/40001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/resolution.dart (right):

https://codereview.chromium.org/1856953003/diff/40001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/resolution.dart:189: /// If set to `true` resolution
caches will not be cleared. Use the only for
the only => only

https://codereview.chromium.org/1856953003/diff/40001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/resolution.dart:191: bool retainCaches;
since Dart doesn't have annotations for testing, I often name these kind of
variables with a testing name. For example: retainCachesForTesting.

https://codereview.chromium.org/1856953003/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/serialization_model_test.dart (right):

https://codereview.chromium.org/1856953003/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_model_test.dart:173: for
(ClassHierarchyNode other in b.directSubclasses) {
what do we need to do to guarantee that directSubclasses are always
serialized/deserialized in the same order? If we already do, let's change our
tests to also verify that (if anything the logic here should get simpler).

(my motivation: having stability is going to be really important to get good
caching behavior when we are working on top of build systems like make and
bazel, it also makes it easier for users to inspect by hand what was the effect
from a small change in their program)

https://codereview.chromium.org/1856953003/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_model_test.dart:201: for (var element2 in
remaining) {
(a) seems that there is a lot in common between this and the other equivalent
tests you had from the other change? Can they be merged?

(b) I know this is just for tests, so this is low priority, but I wonder if we
can avoid the quadratic cost here. Some ideas:
 - convert each set to a list and sort it first, then compare by walking both
lists in parallel
 - map each element to a canonical object, create a map containing those
mappings, use the mapped sets to compare (then operations like set.difference
would work)

[Johnni Winther, 2016-04-06 09:30:26]

Description was changed from

==========
Test closed world model after deserialization.

We still need better tracking of native/foreign elements.
==========

to

==========
Test closed world model after deserialization.

We still need better tracking of native/foreign elements.

R=sigmund@google.com

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

[Johnni Winther, 2016-04-06 09:30:27]

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

[Johnni Winther, 2016-04-20 10:12:36]

https://codereview.chromium.org/1856953003/diff/40001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/resolution.dart (right):

https://codereview.chromium.org/1856953003/diff/40001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/resolution.dart:189: /// If set to `true` resolution
caches will not be cleared. Use the only for
On 2016/04/05 15:19:30, Siggi Cherem (dart-lang) wrote:
> the only => only

Done.

https://codereview.chromium.org/1856953003/diff/40001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/resolution.dart:191: bool retainCaches;
On 2016/04/05 15:19:30, Siggi Cherem (dart-lang) wrote:
> since Dart doesn't have annotations for testing, I often name these kind of
> variables with a testing name. For example: retainCachesForTesting.

Done.

https://codereview.chromium.org/1856953003/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/serialization_model_test.dart (right):

https://codereview.chromium.org/1856953003/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_model_test.dart:173: for
(ClassHierarchyNode other in b.directSubclasses) {
On 2016/04/05 15:19:30, Siggi Cherem (dart-lang) wrote:
> what do we need to do to guarantee that directSubclasses are always
> serialized/deserialized in the same order? If we already do, let's change our
> tests to also verify that (if anything the logic here should get simpler).
> 
> (my motivation: having stability is going to be really important to get good
> caching behavior when we are working on top of build systems like make and
> bazel, it also makes it easier for users to inspect by hand what was the
effect
> from a small change in their program)

Adding a TODO. [ClassHierarchyNode.directSubclasses] must be deterministic
across serialization (for the instantiated classes at least)

https://codereview.chromium.org/1856953003/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_model_test.dart:201: for (var element2 in
remaining) {
On 2016/04/05 15:19:30, Siggi Cherem (dart-lang) wrote:
> (a) seems that there is a lot in common between this and the other equivalent
> tests you had from the other change? Can they be merged?
> 
> (b) I know this is just for tests, so this is low priority, but I wonder if we
> can avoid the quadratic cost here. Some ideas:
>  - convert each set to a list and sort it first, then compare by walking both
> lists in parallel
>  - map each element to a canonical object, create a map containing those
> mappings, use the mapped sets to compare (then operations like set.difference
> would work)

(a) Done.
(b) Added as a TODO.