Serialize TreeElements

Serialize TreeElements

R=sigmund@google.com

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

[Johnni Winther, 2016-04-08 15:25:40]

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

[Johnni Winther, 2016-04-08 15:25:40]

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization_impact_test.dart (right):

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization_impact_test.dart:50:
checkAllImpacts(compilerNormal, compilerDeserialized, verbose: true);
From https://codereview.chromium.org/1870133002/

[Siggi Cherem (dart-lang), 2016-04-08 16:55:33]

nice! lgtm

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
pkg/compiler/lib/src/library_loader.dart:655: return
deserializer.readLibrary(resolvedUri).then((LibraryElement library) {
or use `await`?

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

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:80:
elements.analyzedElement.compilationUnit.script.resourceUri,
eventually we should look into what URI to use - readableUri might be closer to
what we need. Basically, I believe we need something that is not absolute for
one person's file-system (so the serialized data is transferable to other
machines)

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:97: } else if
(element.isFactoryConstructor) {
nit: small refactor just to make it more self explanatory:

  } else {
    // element has a body that we'll need to re-parse. We store where to start
parsing from.
    objectEncoder.setInt(Key.OFFSET, root.getBeginToken().charOffset);

    if (element.isFactoryConstructor) {
      kind = AstKind.FACTORY;
    } else if (element.isField) {
      kind = AstKind.FIELD;
    } else {
      kind = AstKind.FUCTION;
      ...
    }

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:101:
objectEncoder.setInt(Key.OFFSET, root.getBeginToken().charOffset);
question: as we make progress we'll drop the token stream as soon as we are done
parsing. At that point, can we encode enough data using source-information to
determine the offset we need?

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:109:
Key.GET_OR_SET, functionExpression.getOrSet.charOffset);
follow up question: this seems trickier to do without the token stream. One
option is to save that directly as part of the source-information too, another
option would be to change the parser slightly. For example, we can tell it that
we expect a get or set, but don't provide the token itself.

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization_helper.dart (right):

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization_helper.dart:66: String serializedData,
reminder to run dartfmt before submitting :)

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization_helper.dart:137: Future<LibraryElement>
readLibrary(Uri resolvedUri) {
consider using async for this body?

[Johnni Winther, 2016-04-11 08:54:04]

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
pkg/compiler/lib/src/library_loader.dart:655: return
deserializer.readLibrary(resolvedUri).then((LibraryElement library) {
On 2016/04/08 16:55:32, Siggi Cherem (dart-lang) wrote:
> or use `await`?

Aren't we still avoiding the using of async/await for the dartino self-test?

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

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:80:
elements.analyzedElement.compilationUnit.script.resourceUri,
On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> eventually we should look into what URI to use - readableUri might be closer
to
> what we need. Basically, I believe we need something that is not absolute for
> one person's file-system (so the serialized data is transferable to other
> machines)

This is a known issue for the serialization itself. It needs to always serialize
relative to some abstract base (like library root, package roots, current
working directory). With such ability, resourceUri will remain valid.

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:97: } else if
(element.isFactoryConstructor) {
On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> nit: small refactor just to make it more self explanatory:
> 
>   } else {
>     // element has a body that we'll need to re-parse. We store where to start
> parsing from.
>     objectEncoder.setInt(Key.OFFSET, root.getBeginToken().charOffset);
> 
>     if (element.isFactoryConstructor) {
>       kind = AstKind.FACTORY;
>     } else if (element.isField) {
>       kind = AstKind.FIELD;
>     } else {
>       kind = AstKind.FUCTION;
>       ...
>     }

Done.

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:101:
objectEncoder.setInt(Key.OFFSET, root.getBeginToken().charOffset);
On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> question: as we make progress we'll drop the token stream as soon as we are
done
> parsing. At that point, can we encode enough data using source-information to
> determine the offset we need?

Yes. Source information such replace tokens/nodes for offset info.

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:109:
Key.GET_OR_SET, functionExpression.getOrSet.charOffset);
On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> follow up question: this seems trickier to do without the token stream. One
> option is to save that directly as part of the source-information too, another
> option would be to change the parser slightly. For example, we can tell it
that
> we expect a get or set, but don't provide the token itself.

This is only for reparsing. When we have a complete serialized resolved AST we
don't need it.

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization_helper.dart (right):

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization_helper.dart:66: String serializedData,
On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> reminder to run dartfmt before submitting :)

I think we need to on all unittests....

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization_helper.dart:137: Future<LibraryElement>
readLibrary(Uri resolvedUri) {
On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> consider using async for this body?

This is a precursor the 'actual' deserializer so I kept it in 'dart2js-style',
i.e. (still) not using async/await.

[Johnni Winther, 2016-04-11 09:16:16]

Description was changed from

==========
Serialize TreeElements
==========

to

==========
Serialize TreeElements

R=sigmund@google.com

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

[Johnni Winther, 2016-04-11 09:16:18]

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

[Siggi Cherem (dart-lang), 2016-04-11 17:48:07]

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
pkg/compiler/lib/src/library_loader.dart:655: return
deserializer.readLibrary(resolvedUri).then((LibraryElement library) {
On 2016/04/11 08:54:04, Johnni Winther wrote:
> On 2016/04/08 16:55:32, Siggi Cherem (dart-lang) wrote:
> > or use `await`?
> 
> Aren't we still avoiding the using of async/await for the dartino self-test?

are they still blocked on that? I thought it was going to take about a month or
so to get past that.

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

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:80:
elements.analyzedElement.compilationUnit.script.resourceUri,
On 2016/04/11 08:54:04, Johnni Winther wrote:
> On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> > eventually we should look into what URI to use - readableUri might be closer
> to
> > what we need. Basically, I believe we need something that is not absolute
for
> > one person's file-system (so the serialized data is transferable to other
> > machines)
> 
> This is a known issue for the serialization itself. It needs to always
serialize
> relative to some abstract base (like library root, package roots, current
> working directory). With such ability, resourceUri will remain valid.

could we file a bug/add a todo to track this issue explicitly?

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization_helper.dart (right):

https://codereview.chromium.org/1873573004/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization_helper.dart:66: String serializedData,
On 2016/04/11 08:54:04, Johnni Winther wrote:
> On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> > reminder to run dartfmt before submitting :)
> 
> I think we need to on all unittests....

Ah - good point, I didn't realize this was on the test side of thigns

[Johnni Winther, 2016-04-12 08:05:47]

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/librar...
pkg/compiler/lib/src/library_loader.dart:655: return
deserializer.readLibrary(resolvedUri).then((LibraryElement library) {
On 2016/04/11 17:48:07, Siggi Cherem (dart-lang) wrote:
> On 2016/04/11 08:54:04, Johnni Winther wrote:
> > On 2016/04/08 16:55:32, Siggi Cherem (dart-lang) wrote:
> > > or use `await`?
> > 
> > Aren't we still avoiding the using of async/await for the dartino self-test?
> 
> are they still blocked on that? I thought it was going to take about a month
or
> so to get past that.

I'll check with sgjesse and let you know.

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

https://codereview.chromium.org/1873573004/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:80:
elements.analyzedElement.compilationUnit.script.resourceUri,
On 2016/04/11 17:48:07, Siggi Cherem (dart-lang) wrote:
> On 2016/04/11 08:54:04, Johnni Winther wrote:
> > On 2016/04/08 16:55:33, Siggi Cherem (dart-lang) wrote:
> > > eventually we should look into what URI to use - readableUri might be
closer
> > to
> > > what we need. Basically, I believe we need something that is not absolute
> for
> > > one person's file-system (so the serialized data is transferable to other
> > > machines)
> > 
> > This is a known issue for the serialization itself. It needs to always
> serialize
> > relative to some abstract base (like library root, package roots, current
> > working directory). With such ability, resourceUri will remain valid.
> 
> could we file a bug/add a todo to track this issue explicitly?

Done: #26244