Support serialization of WorldImpact

Support serialization of WorldImpact

R=sigmund@google.com

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

[Johnni Winther, 2016-03-16 15:25:27]

Description was changed from

==========
Serialize WorldImpact

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

to

==========
Support serialization of WorldImpact
==========

[Johnni Winther, 2016-03-16 15:25:36]

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

[Siggi Cherem (dart-lang), 2016-03-16 23:40:14]

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

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:621:
element.sourcePosition.uri.path.contains('_internal/js_runtime/lib/')) ||
nit: 80 (same below)

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:621:
element.sourcePosition.uri.path.contains('_internal/js_runtime/lib/')) ||
do we have guarantees that the uri will contain that?

In particular, if serialization merges the library + the patch, are we picking
the uri from the patch and not the library?

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:635: assert(invariant(element,
false,
Seems like the `registerUse` logic is mainly for this assertion, should we
change this code to only do this detailed checks in the assertion itself? That
would also make the register method simpler. For example:

  void registerBackendUse(Element element) {
    if (element == null) return;
    assert(invariant(element, _isValidBackendUse(element), ...);
    helpersUsed.add(element.declaration);
    if (element.isClass && element.isPatched) {
      helpersUsed.add(element.implementation);
    }
  }
  
  bool _isValidBackendUse(Element element) =>
      // TODO(johnniwinther): These are valid but we could be more precise.
      element == helpers.streamIteratorConstructor ||
      element == helpers.compiler.symbolConstructor ||
      element == helpers.compiler.symbolValidatedConstructor ||
      element == helpers.syncCompleterConstructor ||
      element == coreClasses.symbolClass ||
      element == helpers.objectNoSuchMethod ||
      // TODO(johnniwinther): We should be more precise about these.
      element.implementationLibrary.isPatch ||
      element.library == helpers.jsHelperLibrary ||
      element.library == helpers.interceptorsLibrary ||
      element.library == helpers.isolateHelperLibrary ||
      // TODO(johnniwinther): Avoid these.
      element == coreClasses.listClass ||
      element == helpers.mapLiteralClass ||
      element == coreClasses.functionClass ||
      element == coreClasses.stringClass;

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

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_serialization.dart:35: void
onElement(Element element, ObjectEncoder createEncoder(String tag)) {
// TODO: add other pieces of native data? (e.g. js-interop names, etc)

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

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/native_data.dart:14: class NativeData {
+doc on this class

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/serialization.dart (right):

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/serialization.dart:874: if (pluginData !=
null) {
simplify with "?." :) 

  plugin.onElement(element,
      (t) => pluginData?.getObject(t, isOptional:true));

[Johnni Winther, 2016-03-17 10:49:37]

PTAL

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

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:621:
element.sourcePosition.uri.path.contains('_internal/js_runtime/lib/')) ||
On 2016/03/16 23:40:14, Siggi Cherem (dart-lang) wrote:
> nit: 80 (same below)

Done.

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:621:
element.sourcePosition.uri.path.contains('_internal/js_runtime/lib/')) ||
On 2016/03/16 23:40:14, Siggi Cherem (dart-lang) wrote:
> do we have guarantees that the uri will contain that?
> 
> In particular, if serialization merges the library + the patch, are we picking
> the uri from the patch and not the library?
> 

This is not for patch elements (we must register the declaration element), but
for injected elements, i.e. elements declared in the patch files.

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:635: assert(invariant(element,
false,
On 2016/03/16 23:40:14, Siggi Cherem (dart-lang) wrote:
> Seems like the `registerUse` logic is mainly for this assertion, should we
> change this code to only do this detailed checks in the assertion itself? That
> would also make the register method simpler. For example:
> 
>   void registerBackendUse(Element element) {
>     if (element == null) return;
>     assert(invariant(element, _isValidBackendUse(element), ...);
>     helpersUsed.add(element.declaration);
>     if (element.isClass && element.isPatched) {
>       helpersUsed.add(element.implementation);
>     }
>   }
>   
>   bool _isValidBackendUse(Element element) =>
>       // TODO(johnniwinther): These are valid but we could be more precise.
>       element == helpers.streamIteratorConstructor ||
>       element == helpers.compiler.symbolConstructor ||
>       element == helpers.compiler.symbolValidatedConstructor ||
>       element == helpers.syncCompleterConstructor ||
>       element == coreClasses.symbolClass ||
>       element == helpers.objectNoSuchMethod ||
>       // TODO(johnniwinther): We should be more precise about these.
>       element.implementationLibrary.isPatch ||
>       element.library == helpers.jsHelperLibrary ||
>       element.library == helpers.interceptorsLibrary ||
>       element.library == helpers.isolateHelperLibrary ||
>       // TODO(johnniwinther): Avoid these.
>       element == coreClasses.listClass ||
>       element == helpers.mapLiteralClass ||
>       element == coreClasses.functionClass ||
>       element == coreClasses.stringClass;

Good point. Done.

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

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_serialization.dart:35: void
onElement(Element element, ObjectEncoder createEncoder(String tag)) {
On 2016/03/16 23:40:14, Siggi Cherem (dart-lang) wrote:
> // TODO: add other pieces of native data? (e.g. js-interop names, etc)

Done.

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

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/native_data.dart:14: class NativeData {
On 2016/03/16 23:40:14, Siggi Cherem (dart-lang) wrote:
> +doc on this class

Done.

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/serialization.dart (right):

https://codereview.chromium.org/1809533004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/serialization.dart:874: if (pluginData !=
null) {
On 2016/03/16 23:40:14, Siggi Cherem (dart-lang) wrote:
> simplify with "?." :) 
> 
>   plugin.onElement(element,
>       (t) => pluginData?.getObject(t, isOptional:true));

Done.

[Siggi Cherem (dart-lang), 2016-03-17 15:33:45]

lgtm

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

https://codereview.chromium.org/1809533004/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:631: } else if
(element.implementationLibrary.isPatch ||
nit, can be done later: any reason why not merge all the conditions as a single
expression ||? The current structure of if-else makes it seem like there is more
to it than just a list of possible cases.

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

https://codereview.chromium.org/1809533004/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_test.dart:461: if (element1 == null &&
element2 == null) return;
wouldn't this be covered by the condition below? or did you mean "||"?

[Johnni Winther, 2016-03-18 08:05:35]

Description was changed from

==========
Support serialization of WorldImpact
==========

to

==========
Support serialization of WorldImpact

R=sigmund@google.com

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

[Johnni Winther, 2016-03-18 08:05:37]

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

[Johnni Winther, 2016-03-18 08:11:32]

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

https://codereview.chromium.org/1809533004/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:631: } else if
(element.implementationLibrary.isPatch ||
On 2016/03/17 15:33:45, Siggi Cherem (dart-lang) wrote:
> nit, can be done later: any reason why not merge all the conditions as a
single
> expression ||? The current structure of if-else makes it seem like there is
more
> to it than just a list of possible cases.

They are grouped by the TODOs (the reasons for allowing their backend use).

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

https://codereview.chromium.org/1809533004/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_test.dart:461: if (element1 == null &&
element2 == null) return;
On 2016/03/17 15:33:45, Siggi Cherem (dart-lang) wrote:
> wouldn't this be covered by the condition below? or did you mean "||"?

Moved `if (element1 == element2) ...` below `e = e.declaration`.