Support (de)serialization from command-line

Support (de)serialization from command-line

R=sigmund@google.com

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

[Johnni Winther, 2016-05-13 12:36:19]

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

[Johnni Winther, 2016-05-13 12:36:20]

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

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/system.dart:139: 
This is just moved from above the DeserializerSystem

[Siggi Cherem (dart-lang), 2016-05-14 01:10:31]

very cool Johnni!

Let's chat about the flags some more before we land it. I included some ideas
below.

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:62: // Experimental options.
Several ideas/comments about how to expose the flags:

a. "*-source": it would be good to support a list of sources right away :)

b. "*-target": let's make the feature itself a boolean flag instead, and
overload the `--out` flag to specify the output file if so desired.

c. let's use a word that better describe the feature we are exposing. So instead
of "serialization", maybe we could use refer to "resolution" or the fact that
this is lowering to a shared ir? For example:
  --emit-resolved-data
  --resolve-only
  --output=ir // overload the output=dart flag we used to have :)
  --summarize // this is the term used by dart-analyzer/ddc

Other ideas? Currently `--resolve-only` is the one that is winning for me. I
guess later on we will have separate binaries, so the flags can be more
straightforward.


Putting this together I can imagine an API like this:

- compile a.dart into a.js:
    dart2js a.dart

- compile a.dart into foo.js:
    dart2js -o foo.js a.dart

- emit the resolution data for a.dart into a.data
    dart2js --resolve-only a.dart

- emit the resolution data for a.dart into foo.data
    dart2js --resolve-only -o foo.data a.dart

- emit the resolution data for a.dart and b.dart into foo.data
    dart2js --resolve-only -o foo.data a.dart b.dart

- use resolution data from i.data and j.data
    dart2js --resolve-only --input=i.data,j.data -o foo.data a.dart b.dart

 Or, we could overload the list of arguments too, for example:

    dart2js --resolve-only -o foo.data a.dart b.dart i.data j.data

Thoughts?

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/dart2j...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/dart2j...
pkg/compiler/lib/src/dart2js.dart:466: helpAndFail("Option
'${Option.serializationTarget}' is only supported "
I'd be OK implying it (instead of requiring that we have to write --analyze-all
as well.)

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

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/task.dart:31: DeserializerSystem system;
I sort of prefer the old name here.

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/source...
File pkg/compiler/lib/src/source_file_provider.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/source...
pkg/compiler/lib/src/source_file_provider.dart:290: if (serializationTarget ==
null) {
what if they specify serializationTarget with a different extension?

Should we have a default way to serialize without providing a name (just use
out.data?)

https://codereview.chromium.org/1975153002/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/analysis_test.dart (right):

https://codereview.chromium.org/1975153002/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/analysis_test.dart:15: import
'package:compiler/src/serialization/system.dart';
unused import? 

(same thing in the next few files)

[Johnni Winther, 2016-05-17 12:37:34]

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:62: // Experimental options.
On 2016/05/14 01:10:31, Siggi Cherem (dart-lang) wrote:
> Several ideas/comments about how to expose the flags:
> 
> a. "*-source": it would be good to support a list of sources right away :)
> 
> b. "*-target": let's make the feature itself a boolean flag instead, and
> overload the `--out` flag to specify the output file if so desired.
> 
> c. let's use a word that better describe the feature we are exposing. So
instead
> of "serialization", maybe we could use refer to "resolution" or the fact that
> this is lowering to a shared ir? For example:
>   --emit-resolved-data
>   --resolve-only
>   --output=ir // overload the output=dart flag we used to have :)
>   --summarize // this is the term used by dart-analyzer/ddc
> 
> Other ideas? Currently `--resolve-only` is the one that is winning for me. I
> guess later on we will have separate binaries, so the flags can be more
> straightforward.
> 
> 
> Putting this together I can imagine an API like this:
> 
> - compile a.dart into a.js:
>     dart2js a.dart
> 
> - compile a.dart into foo.js:
>     dart2js -o foo.js a.dart
> 
> - emit the resolution data for a.dart into a.data
>     dart2js --resolve-only a.dart
> 
> - emit the resolution data for a.dart into foo.data
>     dart2js --resolve-only -o foo.data a.dart
> 
> - emit the resolution data for a.dart and b.dart into foo.data
>     dart2js --resolve-only -o foo.data a.dart b.dart
> 
> - use resolution data from i.data and j.data
>     dart2js --resolve-only --input=i.data,j.data -o foo.data a.dart b.dart
> 
>  Or, we could overload the list of arguments too, for example:
> 
>     dart2js --resolve-only -o foo.data a.dart b.dart i.data j.data
> 
> Thoughts?

The --serialization-X are not meant to last but we something to start testing.
(That's why I put them under 'experimental options' and did not document them in
the help text in 'dart2js.dart')

Regarding the actual (user-facing) command line options we still need to figure
out good strategies for sharding (dart:* in one file, each package in its one,
etc.), ability to autoload data (deploying the sdk with data for dart:*, loading
data if present (and up-to-date) and otherwise use .dart files (maybe autosave
as well).

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/dart2j...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/dart2j...
pkg/compiler/lib/src/dart2js.dart:466: helpAndFail("Option
'${Option.serializationTarget}' is only supported "
On 2016/05/14 01:10:31, Siggi Cherem (dart-lang) wrote:
> I'd be OK implying it (instead of requiring that we have to write
--analyze-all
> as well.)

Done.

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

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/task.dart:31: DeserializerSystem system;
On 2016/05/14 01:10:31, Siggi Cherem (dart-lang) wrote:
> I sort of prefer the old name here.

Done.

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/source...
File pkg/compiler/lib/src/source_file_provider.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/source...
pkg/compiler/lib/src/source_file_provider.dart:290: if (serializationTarget ==
null) {
On 2016/05/14 01:10:31, Siggi Cherem (dart-lang) wrote:
> what if they specify serializationTarget with a different extension?
> 
> Should we have a default way to serialize without providing a name (just use
> out.data?)

[extension] is a bad name; it is more like a 'mime-type'. [serializationTarget]
can be anything.

Yes. I think we should have a default (together with the autoload/save)
capability.

https://codereview.chromium.org/1975153002/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/analysis_test.dart (right):

https://codereview.chromium.org/1975153002/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/analysis_test.dart:15: import
'package:compiler/src/serialization/system.dart';
On 2016/05/14 01:10:31, Siggi Cherem (dart-lang) wrote:
> unused import? 
> 
> (same thing in the next few files)

So it was...

[Siggi Cherem (dart-lang), 2016-05-17 21:54:26]

lgtm

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/1975153002/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:62: // Experimental options.
On 2016/05/17 12:37:33, Johnni Winther wrote:
> On 2016/05/14 01:10:31, Siggi Cherem (dart-lang) wrote:
> > Several ideas/comments about how to expose the flags:
> > 
> > a. "*-source": it would be good to support a list of sources right away :)
> > 
> > b. "*-target": let's make the feature itself a boolean flag instead, and
> > overload the `--out` flag to specify the output file if so desired.
> > 
> > c. let's use a word that better describe the feature we are exposing. So
> instead
> > of "serialization", maybe we could use refer to "resolution" or the fact
that
> > this is lowering to a shared ir? For example:
> >   --emit-resolved-data
> >   --resolve-only
> >   --output=ir // overload the output=dart flag we used to have :)
> >   --summarize // this is the term used by dart-analyzer/ddc
> > 
> > Other ideas? Currently `--resolve-only` is the one that is winning for me. I
> > guess later on we will have separate binaries, so the flags can be more
> > straightforward.
> > 
> > 
> > Putting this together I can imagine an API like this:
> > 
> > - compile a.dart into a.js:
> >     dart2js a.dart
> > 
> > - compile a.dart into foo.js:
> >     dart2js -o foo.js a.dart
> > 
> > - emit the resolution data for a.dart into a.data
> >     dart2js --resolve-only a.dart
> > 
> > - emit the resolution data for a.dart into foo.data
> >     dart2js --resolve-only -o foo.data a.dart
> > 
> > - emit the resolution data for a.dart and b.dart into foo.data
> >     dart2js --resolve-only -o foo.data a.dart b.dart
> > 
> > - use resolution data from i.data and j.data
> >     dart2js --resolve-only --input=i.data,j.data -o foo.data a.dart b.dart
> > 
> >  Or, we could overload the list of arguments too, for example:
> > 
> >     dart2js --resolve-only -o foo.data a.dart b.dart i.data j.data
> > 
> > Thoughts?
> 
> The --serialization-X are not meant to last but we something to start testing.
> (That's why I put them under 'experimental options' and did not document them
in
> the help text in 'dart2js.dart')
> 

I understand, I was also thinking we'd keep the new flags in experimental. It
might still be worth using flags closer to how we imagine them to be in the
future, though. For example, how about this combination:
  - Use `--resolve-only` to enable serialization (this will be an experimental
flag added here)
  - Remove `--serialization-target` and use `--out` (with a default)
  - keep the `--serialization-source` extra flag, maybe rename to
`--resolution-input` to match the other flag name. 

> Regarding the actual (user-facing) command line options we still need to
figure
> out good strategies for sharding (dart:* in one file, each package in its one,
> etc.), ability to autoload data (deploying the sdk with data for dart:*,
loading
> data if present (and up-to-date) and otherwise use .dart files (maybe autosave
> as well).

Agree, all good questions. My thinking on this:
 - re sharding: I'm inclined to be explicit about it on the command line, for
example: specify which files should go into the same output. For example:

  dart2js --resolve-only --resolution-input=a.z --out=o.z b.dart c.dart

will include B and C in the output o.z file.

 - re file is present/not-present: I'm inclined to keep it simple, if it is not
in the input list and it is not loaded in an existing serialized chunk, then we
issue an error. In other words: 

   In the example above if b.dart imports c.dart and a.dart, then this fails
because c is not found (the file should've been given as an input like the
example above)

    dart2js --resolve-only --resolution-input=a.z --out=o.z b.dart

  this also fails, because a's resolution input is missing:
    dart2js --resolve-only --out=o.z b.dart c.dart

https://codereview.chromium.org/1975153002/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1975153002/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart2js.dart:455: }
maybe set analyzeOnly here too?

That way we don't report a chain of warnings like:

  Option --serialization-target implies --analyze-all
  Option --analyze-all implies --analyze-only

:)

[Johnni Winther, 2016-05-18 09:52:37]

Description was changed from

==========
Support (de)serialization from command-line
==========

to

==========
Support (de)serialization from command-line

R=sigmund@google.com

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

[Johnni Winther, 2016-05-18 09:52:41]

Committed patchset #3 (id:40001) manually as
6571bdb95e32b91c99d9017bbb9b99c7d4bda793 (presubmit successful).

[Johnni Winther, 2016-05-18 09:52:41]

PTAL at the last patch (post commit)

https://codereview.chromium.org/1975153002/diff/20001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1975153002/diff/20001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart2js.dart:455: }
On 2016/05/17 21:54:25, Siggi Cherem (dart-lang) wrote:
> maybe set analyzeOnly here too?
> 
> That way we don't report a chain of warnings like:
> 
>   Option --serialization-target implies --analyze-all
>   Option --analyze-all implies --analyze-only
> 
> :)

Done.

[Siggi Cherem (dart-lang), 2016-05-18 17:58:39]

lgtm, thanks Johnni!