Rework DillLoader to allow adding multiple dills.

Rework DillLoader to allow adding multiple dills.

This allows us to compose a consistent bundle with SDK and the transitive
closure of required libraries for a libarary, and then compile it
separately (or as a part of its own library cycle).

There is still more data public than I'd like, but I will leave this
clean up fo later.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Siggi Cherem (dart-lang), 2017-05-09 20:14:30]

Thanks for doing this!

I have a few API suggestions and minor nits.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:16: Program program;
doesn't have to be in this CL, but we probably only need to store List<Library>
and not Program here. It might be nuance, but it makes it clear to me that this
is just a collection of libraries, not an actual program

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:20: /// Add already compiled
libraries from the given [dill].  If [uris] is
nit: a couple suggestions on this comment:
 - I prefer to refer to the program as [program] rather than [dill]. I've been
thinking that maybe just the class should use the term dill, I might send you a
follow up CL with more docs :)
 - I think you are missing a "not" (if uris is *not* null)
 - I would remove the note about dependencies - it matters that all dependencies
are added before running the kernel target/builders, but I don't believe it
matters in what order you add programs here (e.g. a dependency can come before
or after). 

I have a couple more suggestions below about the API, so all-together this might
be more like:

/// Append compiled libraries from the given [program]. If [filter]
/// is provided, this only adds libraries whose Uri is accepted by
/// such filter.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:25: void addDill(Program dill,
[Set<Uri> uris]) {
some suggestions:
 - rename addDill => appendProgram or addLibrariesFromProgram
 - dill => program

change [Set<Uri> uris] into a filter callback: that way you don't force a
representation of the filter to the clients of this API:

   addLibrariesFromProgram(Program program, [bool filter(Uri uri)])

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:48: void setProgram(Program
program) {
I'd delete this and use `addDill` in testing/suite.dart instead

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_target.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_target.dart:40: inputError(uri, -1, "Use
appendDillForProgram() instead.");
should this be internalError("not implemented") instead?

alternatively, we could do the append call here.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/fasta.dart:266: /// If the [uri] is not empty, load
a [Program] from it and append its
nit: "empty" => "null"

Do we have any cases where uri is null? or can we get rid of that check?

[scheglov, 2017-05-09 20:32:22]

PTAL

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:16: Program program;
On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> doesn't have to be in this CL, but we probably only need to store
List<Library>
> and not Program here. It might be nuance, but it makes it clear to me that
this
> is just a collection of libraries, not an actual program

Yeah, mostly we use libraries.
But lib/src/fasta/kernel/kernel_target.dart:371 KernelTarget.link() uses also
binary.uriToSource.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:20: /// Add already compiled
libraries from the given [dill].  If [uris] is
On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> nit: a couple suggestions on this comment:
>  - I prefer to refer to the program as [program] rather than [dill]. I've been
> thinking that maybe just the class should use the term dill, I might send you
a
> follow up CL with more docs :)
>  - I think you are missing a "not" (if uris is *not* null)
>  - I would remove the note about dependencies - it matters that all
dependencies
> are added before running the kernel target/builders, but I don't believe it
> matters in what order you add programs here (e.g. a dependency can come before
> or after). 
> 
> I have a couple more suggestions below about the API, so all-together this
might
> be more like:
> 
> /// Append compiled libraries from the given [program]. If [filter]
> /// is provided, this only adds libraries whose Uri is accepted by
> /// such filter.

Done.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:25: void addDill(Program dill,
[Set<Uri> uris]) {
On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> some suggestions:
>  - rename addDill => appendProgram or addLibrariesFromProgram
>  - dill => program
> 
> change [Set<Uri> uris] into a filter callback: that way you don't force a
> representation of the filter to the clients of this API:
> 
>    addLibrariesFromProgram(Program program, [bool filter(Uri uri)])
> 

Done.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:48: void setProgram(Program
program) {
On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> I'd delete this and use `addDill` in testing/suite.dart instead

Ah... Right, we don't need it anymore!

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_target.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_target.dart:40: inputError(uri, -1, "Use
appendDillForProgram() instead.");
On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> should this be internalError("not implemented") instead?
> 
> alternatively, we could do the append call here.

Done.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/fasta.dart:266: /// If the [uri] is not empty, load
a [Program] from it and append its
On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> nit: "empty" => "null"
> 
> Do we have any cases where uri is null? or can we get rid of that check?

I think we don't.
In one case the "platform" is a named parameter, but it seems always filled.
I removed the check.

[Siggi Cherem (dart-lang), 2017-05-09 22:12:37]

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

[Siggi Cherem (dart-lang), 2017-05-09 22:12:37]

+Kevin for changes to kernel.

Changes to dill loader LGTM, I have one follow-up question/suggestion below
based on our conversation this afternoon, I'm ok addressing that separately.

Please wait for Kevin or Peter to approve the change to pkg/kernel.

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:20: /// Add already compiled
libraries from the given [dill].  If [uris] is
On 2017/05/09 20:32:21, scheglov wrote:
> On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> > nit: a couple suggestions on this comment:
> >  - I prefer to refer to the program as [program] rather than [dill]. I've
been
> > thinking that maybe just the class should use the term dill, I might send
you
> a
> > follow up CL with more docs :)
> >  - I think you are missing a "not" (if uris is *not* null)
> >  - I would remove the note about dependencies - it matters that all
> dependencies
> > are added before running the kernel target/builders, but I don't believe it
> > matters in what order you add programs here (e.g. a dependency can come
before
> > or after). 
> > 
> > I have a couple more suggestions below about the API, so all-together this
> might
> > be more like:
> > 
> > /// Append compiled libraries from the given [program]. If [filter]
> > /// is provided, this only adds libraries whose Uri is accepted by
> > /// such filter.
> 
> Done.

After talking on VC I just realized something: is filter mainly used to exclude
dependencies?

For example:
// load sdk dill:
 appendLibraries(sdkDill);

// load library that uses the sdk:
 appendLibraries(packageFooDill, filter(uri) =>
'$uri'.startsWith('package:foo'))?

If we have a modular .dill for this package, all sdk dependencies will be marked
"external", so another option is to filter based on that. If you think that will
handle all cases where we need the filter, then we could remove the extra
argument and instead do:

   if (!library.isExternal) {
     ...
   }


what do you think?

[scheglov, 2017-05-10 00:47:41]

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:20: /// Add already compiled
libraries from the given [dill].  If [uris] is
On 2017/05/09 22:12:37, Siggi Cherem (dart-lang) wrote:
> On 2017/05/09 20:32:21, scheglov wrote:
> > On 2017/05/09 20:14:30, Siggi Cherem (dart-lang) wrote:
> > > nit: a couple suggestions on this comment:
> > >  - I prefer to refer to the program as [program] rather than [dill]. I've
> been
> > > thinking that maybe just the class should use the term dill, I might send
> you
> > a
> > > follow up CL with more docs :)
> > >  - I think you are missing a "not" (if uris is *not* null)
> > >  - I would remove the note about dependencies - it matters that all
> > dependencies
> > > are added before running the kernel target/builders, but I don't believe
it
> > > matters in what order you add programs here (e.g. a dependency can come
> before
> > > or after). 
> > > 
> > > I have a couple more suggestions below about the API, so all-together this
> > might
> > > be more like:
> > > 
> > > /// Append compiled libraries from the given [program]. If [filter]
> > > /// is provided, this only adds libraries whose Uri is accepted by
> > > /// such filter.
> > 
> > Done.
> 
> After talking on VC I just realized something: is filter mainly used to
exclude
> dependencies?
> 
> For example:
> // load sdk dill:
>  appendLibraries(sdkDill);
> 
> // load library that uses the sdk:
>  appendLibraries(packageFooDill, filter(uri) =>
> '$uri'.startsWith('package:foo'))?
> 
> If we have a modular .dill for this package, all sdk dependencies will be
marked
> "external", so another option is to filter based on that. If you think that
will
> handle all cases where we need the filter, then we could remove the extra
> argument and instead do:
> 
>    if (!library.isExternal) {
>      ...
>    }
> 
> 
> what do you think?

If the idea with serializing just individual strongly linked components of
libraries (without external libraries) works, we might get rid of the filter
here (it will be in BinaryPrinter instead).

[Kevin Millikin (Google), 2017-05-11 09:33:29]

Kernel changes LGTM.

[ahe, 2017-05-11 10:09:49]

lgtm

Nice!

https://codereview.chromium.org/2872903005/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_target.dart (right):

https://codereview.chromium.org/2872903005/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_target.dart:40: internalError("not
implemented");
"Internal error: Unsupported operation."

https://codereview.chromium.org/2872903005/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2872903005/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:252: ticker.logMs("Read platform file");
I don't think this is correct. We've used read the packages file, not the
platform file. The platform file isn't read until line 254.

https://codereview.chromium.org/2872903005/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures/suite.dart (right):

https://codereview.chromium.org/2872903005/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures/suite.dart:64: return
loadProgramFromBinary(platformPath);
Why this change?

[scheglov, 2017-05-11 15:53:11]

https://codereview.chromium.org/2872903005/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2872903005/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:252: ticker.logMs("Read platform file");
On 2017/05/11 10:09:49, ahe wrote:
> I don't think this is correct. We've used read the packages file, not the
> platform file. The platform file isn't read until line 254.

I will roll this back.
I was confused.

https://codereview.chromium.org/2872903005/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures/suite.dart (right):

https://codereview.chromium.org/2872903005/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures/suite.dart:64: return
loadProgramFromBinary(platformPath);
On 2017/05/11 10:09:49, ahe wrote:
> Why this change?

I will roll this back.

[scheglov, 2017-05-11 15:56:42]

Description was changed from

==========
Rework DillLoader to allow adding multiple dills.

This allows us to compose a consistent bundle with SDK and the transitive
closure of required libraries for a libarary, and then compile it
separately (or as a part of its own library cycle).

There is still more data public than I'd like, but I will leave this
clean up fo later.


R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Rework DillLoader to allow adding multiple dills.

This allows us to compose a consistent bundle with SDK and the transitive
closure of required libraries for a libarary, and then compile it
separately (or as a part of its own library cycle).

There is still more data public than I'd like, but I will leave this
clean up fo later.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-05-11 15:56:43]

Committed patchset #3 (id:40001) manually as
8751b910622ff3b77705289e327071e9225887ec (presubmit successful).

[ahe, 2017-05-19 09:15:04]

https://codereview.chromium.org/2872903005/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:37: Future<Null>
buildOutline(DillLibraryBuilder builder) async {
Please stop sorting methods by name, it produces unnecessary merge conflicts and
makes it harder to use "git blame".

[scheglov, 2017-05-19 16:03:26]

https://codereview.chromium.org/2872903005/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:37: Future<Null>
buildOutline(DillLibraryBuilder builder) async {
On 2017/05/19 09:15:03, ahe wrote:
> Please stop sorting methods by name, it produces unnecessary merge conflicts
and
> makes it harder to use "git blame".

Conflicts will happen only once.
Hopefully not too hard to merge for this small file.

I find it much more consistent to sort everything than to rely on vague and
impossible to get explanation about ad hoc order. Again, help to reduce the
cognitive load - no need to think, just do as everywhere.

[ahe, 2017-05-22 09:01:37]

https://codereview.chromium.org/2872903005/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2872903005/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:37: Future<Null>
buildOutline(DillLibraryBuilder builder) async {
On 2017/05/19 16:03:25, scheglov wrote:
> On 2017/05/19 09:15:03, ahe wrote:
> > Please stop sorting methods by name, it produces unnecessary merge conflicts
> and
> > makes it harder to use "git blame".
> 
> Conflicts will happen only once.
> Hopefully not too hard to merge for this small file.
> 
> I find it much more consistent to sort everything than to rely on vague and
> impossible to get explanation about ad hoc order. Again, help to reduce the
> cognitive load - no need to think, just do as everywhere.

What you personally perceive as reducing cognitive load has exactly the opposite
effect on me. If you feel that we should change our coding style, you should
suggest that we do so, and get agreement.

FWIW, following up on a highly contentious thread about automated formatting
isn't a good way to make such a proposal and not a great way to establish
agreement.