Improve parallelism when loading transformer plugins.

Improve parallelism when loading transformer plugins.

Rather than only looking at the package-level dependency graph, pub will now
examine transformer files' imports to determine dependencies between
transformers. This improves both load times and cycle detection. For example, a
package using polymer transformers can now load them in two phases rather than
four, reducing the load time by about half.

In addition, this gives transformer authors more control over how parallel their
transformer graph is. With some $include/$exclude and import tweaks, most
remaining transformer dependencies can be eliminated.

R=rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37461

[nweiz, 2014-06-16 19:56:44]

Unlike the previous iteration of the transformer-ordering code, I decided to go
with longer, more explicit names rather than definitions like "ordering
dependencies". Hopefully that makes the code clearer.

[Bob Nystrom, 2014-06-17 19:59:25]

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:205: /// If `path` is null, this will
determine which library to load.
Document that this doesn't take into account generated assets and why.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:210: if (fileExists(transformerPath))
return transformerPath;
What about generated assets?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:21: /// represented
internally as a linked list of steps.
This is a bit misleading, since the steps themselves are just strings. It's
probably clearer just to say it's a linked list of exceptions, each describing
one step in the overall error.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:44: return
steps.toList();
Why make a Queue just to convert it to a list?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:60: /// Returns a
copy of [this] with [step] added to the beginning of [steps].
It's not really a copy is it? Maybe, "Returns a new CycleException starting with
[step] followed by this exception's steps."

Also, make this a factory ctor?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:37: var sw =
new Stopwatch()..start();
Delete.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:69: var
packagesToUpdate = unionAll(phase.map((id) =>
This could use some comments.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:104: /// phases
in which each transformer in a given phase can be loaded in parallel.
This is a bit terse. How about something like:

"Given [transformerDependencies] which maps each transformer to the set of
transformers it depends on, determines the order in which the transformers will
be loaded.

This does a topological sort of the packages. When possible, it will load
multiple transformers in parallel if they do not depend on each other. Returns a
list of "phases" where each phase contains a set of transformers that can be
loaded in parallel and the phases must be loaded in order."

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:107: var
phaseNumbers = {};
Doc this.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:113: return
dependencies.isEmpty ? 0 :
You'll want to insert this result in phaseNumbers before returning. Right now,
your cache isn't accomplishing much. :)

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:114:
maxAll(dependencies.map(phaseNumberFor)) + 1;
Won't this stack overflow on a cyclic dependency? If those have already been
determined beforehand, document in the doc comment that this assumes there are
none.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:221: if
(id.package != '\$dart2js') return new Set();
Do we need to handle the magic "$pub" package too?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:20:
/// values are sets representing edges from the given vertex. If there's an edge
Before "If..": "Each vertex is a transformer."

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:56:
/// [_PackageDependencyComputer] that have been loaded.
"]" -> "]s".

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:75:
/// [packageUri] can be safely imported from an external package.
Is a "packageUri" a "package:" URL? If so, mention that. Took me a bit to figure
that out.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:82:
var components = p.split(p.fromUri(packageUri.path));
How will this handle "package:/"?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:87:
fail('A transformer imported unknown package "$packageName".');
At least include which transformer did the importing, and the full URL being
imported.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:131:
for (var dep in package.dependencies) {
This needs to handle dev deps and overrides for the root package, doesn't it?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:135:
throw error.prependStep("$packageName depends on ${dep.name}");
This is nice.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:159:
_loadingPackageComputers.remove(packageName);
Might be good to put this in a finally.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:180:
final _imports = new Map<Uri, Set<Uri>>();
imports -> directives.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:198:
final _transitiveExternalImports = new Map<String, Set<Uri>>();
imports -> directives.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:320:
var imports = _getImports(library);
Ditto.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:329:
continue;
What if a library in package foo imports another library in foo which then
imports something external?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:362:
if (_imports.containsKey(libraryUri)) return _imports[libraryUri];
Why not do this check before looking in _applicableTransformers?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:364:
// If a nonexitent library is imported, it will probably be generated by a
"nonexistent".

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:128: List<UriBasedDirective>
parseImportsAndExports(String contents, {String name}) {
Don't forget parts. :)

How about parseDirectives()?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart:15:

These really needs some documentation explaining what they test. It's
foobarbazqux soup right now. Maybe more explicit package names too?

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/transformers_needed_by_transformers/utils.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/utils.dart:22:
/// matching [expected] when run on the package graph defiend by packages in
"defined"

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/utils.dart:28:
var sw = new Stopwatch()..start();
Delete.

[Bob Nystrom, 2014-06-17 20:00:06]

What kind of load time improvement do you see with this change?

[nweiz, 2014-06-17 23:38:52]

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:205: /// If `path` is null, this will
determine which library to load.
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> Document that this doesn't take into account generated assets and why.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:210: if (fileExists(transformerPath))
return transformerPath;
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> What about generated assets?

This is used for determining transformer dependencies before initializing
barback. If the libraries are modified by transformers, that logic will fall
back to its conservative behavior.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:21: /// represented
internally as a linked list of steps.
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> This is a bit misleading, since the steps themselves are just strings. It's
> probably clearer just to say it's a linked list of exceptions, each describing
> one step in the overall error.

I think this is pretty standard terminology. Each exception is a node, with a
string as its contents. You say "a linked of strings" just like you say "an
array of strings"; it doesn't mean that the nodes themselves are strings, just
that their contents are.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:44: return
steps.toList();
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> Why make a Queue just to convert it to a list?

My instinct is usually to use a queue when adding elements one by one, but I
could make it a list if you think that's clearer.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:60: /// Returns a
copy of [this] with [step] added to the beginning of [steps].
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> It's not really a copy is it? Maybe, "Returns a new CycleException starting
with
> [step] followed by this exception's steps."

This is like other copy-on-write methods in that, from the user's perspective,
it produces an object that's mostly identical to the original with a set of
explicit changes. Usually we describe those methods as "return a copy with
<difference>". The fact that the new object has a reference to the old one is an
implementation detail.

> Also, make this a factory ctor?

That would make the invocations a lot more verbose and I don't think it adds any
value.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:37: var sw =
new Stopwatch()..start();
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> Delete.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:69: var
packagesToUpdate = unionAll(phase.map((id) =>
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> This could use some comments.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:104: /// phases
in which each transformer in a given phase can be loaded in parallel.
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> This is a bit terse. How about something like:

I've expanded this a bit, but by and large I like the original wording.

> "Given [transformerDependencies] which maps each transformer to the set of
> transformers it depends on, determines the order in which the transformers
will
> be loaded.

I don't like moving away from describing [transformerDependencies] as a directed
graph. That's how we describe these structures elsewhere and thinking of them as
graphs is the easiest way and clearest way to understand the operations being
done to them. It also allows us to piggyback on common mathematical and CS
terminology and understanding.

> This does a topological sort of the packages. When possible, it will load
> multiple transformers in parallel if they do not depend on each other. Returns
a
> list of "phases" where each phase contains a set of transformers that can be
> loaded in parallel and the phases must be loaded in order."

Calling this a topological sort is misleading. A topological ordering is
one-dimensional, where this computes a two-dimensional result.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:107: var
phaseNumbers = {};
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> Doc this.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:113: return
dependencies.isEmpty ? 0 :
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> You'll want to insert this result in phaseNumbers before returning. Right now,
> your cache isn't accomplishing much. :)

:p

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:114:
maxAll(dependencies.map(phaseNumberFor)) + 1;
On 2014/06/17 19:59:23, Bob Nystrom wrote:
> Won't this stack overflow on a cyclic dependency? If those have already been
> determined beforehand, document in the doc comment that this assumes there are
> none.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:221: if
(id.package != '\$dart2js') return new Set();
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> Do we need to handle the magic "$pub" package too?

This is handling transformers, not packages.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:20:
/// values are sets representing edges from the given vertex. If there's an edge
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> Before "If..": "Each vertex is a transformer."

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:56:
/// [_PackageDependencyComputer] that have been loaded.
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> "]" -> "]s".

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:75:
/// [packageUri] can be safely imported from an external package.
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> Is a "packageUri" a "package:" URL? If so, mention that. Took me a bit to
figure
> that out.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:82:
var components = p.split(p.fromUri(packageUri.path));
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> How will this handle "package:/"?

It'll fail with 'A transformer imported unknown package "/".'

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:87:
fail('A transformer imported unknown package "$packageName".');
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> At least include which transformer did the importing,

We have no way of telling that here. I could pipe it through, but I don't think
it's worth the effort and complication when I'm planning on handling the TODO
anyway.

> and the full URL being imported.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:131:
for (var dep in package.dependencies) {
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> This needs to handle dev deps and overrides for the root package, doesn't it?

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:135:
throw error.prependStep("$packageName depends on ${dep.name}");
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> This is nice.

:)

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:159:
_loadingPackageComputers.remove(packageName);
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> Might be good to put this in a finally.

There aren't any recoverable exceptions that could go through here.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:180:
final _imports = new Map<Uri, Set<Uri>>();
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> imports -> directives.

I can do that if you want, but given that neither term is strictly accurate
("library" is also a directive, for example) I think "import" makes the logic
surrounding this a lot clearer than "directive" does.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:329:
continue;
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> What if a library in package foo imports another library in foo which then
> imports something external?

That's exactly what this function handles. It traverses all the intra-foo
imports/exports to find all the outward-facing imports/exports.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:362:
if (_imports.containsKey(libraryUri)) return _imports[libraryUri];
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> Why not do this check before looking in _applicableTransformers?

Because [_applicableTransformers] can change over time. Added a comment to that
effect.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:364:
// If a nonexitent library is imported, it will probably be generated by a
On 2014/06/17 19:59:24, Bob Nystrom wrote:
> "nonexistent".

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:128: List<UriBasedDirective>
parseImportsAndExports(String contents, {String name}) {
On 2014/06/17 19:59:25, Bob Nystrom wrote:
> Don't forget parts. :)

Thankfully, parts can't have their own imports or exports, so we can safely
ignore them.

> How about parseDirectives()?

There are directives (e.g. "library") that aren't imports or exports and that we
don't want to parse here.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart:15:

On 2014/06/17 19:59:25, Bob Nystrom wrote:
> These really needs some documentation explaining what they test. It's
> foobarbazqux soup right now.

Done.

> Maybe more explicit package names too?

I'm not sure what names would help here.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/transformers_needed_by_transformers/utils.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/utils.dart:22:
/// matching [expected] when run on the package graph defiend by packages in
On 2014/06/17 19:59:25, Bob Nystrom wrote:
> "defined"

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/utils.dart:28:
var sw = new Stopwatch()..start();
On 2014/06/17 19:59:25, Bob Nystrom wrote:
> Delete.

Done.

[Bob Nystrom, 2014-06-18 18:24:48]

Couple more suggestions then LGTM.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:210: if (fileExists(transformerPath))
return transformerPath;
On 2014/06/17 23:38:50, nweiz wrote:
> On 2014/06/17 19:59:23, Bob Nystrom wrote:
> > What about generated assets?
> 
> This is used for determining transformer dependencies before initializing
> barback. If the libraries are modified by transformers, that logic will fall
> back to its conservative behavior.

Yup. I realized that by the time I got to the end of this patch, but it wasn't
clear looking just at this code.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:44: return
steps.toList();
On 2014/06/17 23:38:50, nweiz wrote:
> On 2014/06/17 19:59:23, Bob Nystrom wrote:
> > Why make a Queue just to convert it to a list?
> 
> My instinct is usually to use a queue when adding elements one by one, 

Adding to a list has the same behavior and performance. I only use a queue when
I'm removing elements from the front one by one.

> but I could make it a list if you think that's clearer.

My point is more than you're doing redundant work here. You create queue, and
then later create a list from it. The initial queue gives you no value.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:180:
final _imports = new Map<Uri, Set<Uri>>();
On 2014/06/17 23:38:51, nweiz wrote:
> On 2014/06/17 19:59:24, Bob Nystrom wrote:
> > imports -> directives.
> 
> I can do that if you want, but given that neither term is strictly accurate
> ("library" is also a directive, for example) I think "import" makes the logic
> surrounding this a lot clearer than "directive" does.

I do think "directive" is clearer than "import". The latter may make people
think this actively does not work with exports and parts, which it does. Library
tags are technically directives too, but I don't think that would confuse
anyone.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:329:
continue;
On 2014/06/17 23:38:51, nweiz wrote:
> On 2014/06/17 19:59:24, Bob Nystrom wrote:
> > What if a library in package foo imports another library in foo which then
> > imports something external?
> 
> That's exactly what this function handles. It traverses all the intra-foo
> imports/exports to find all the outward-facing imports/exports.

Oh, now I get it. Read it wrong the first time.

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:108: /// Given
[transformerDependencies], a directed acyclic xgraph, returns a list of
Long line. Also "xgraph" -> "graph".

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart
(right):

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart:52:
// Even though foo and bar they don't modify pkg/lib/transformer.dart
Remove "they".

[nweiz, 2014-06-18 20:05:33]

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart (right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/cycle_exception.dart:44: return
steps.toList();
On 2014/06/18 18:24:48, Bob Nystrom wrote:
> On 2014/06/17 23:38:50, nweiz wrote:
> > On 2014/06/17 19:59:23, Bob Nystrom wrote:
> > > Why make a Queue just to convert it to a list?
> > 
> > My instinct is usually to use a queue when adding elements one by one, 
> 
> Adding to a list has the same behavior and performance. I only use a queue
when
> I'm removing elements from the front one by one.
> 
> > but I could make it a list if you think that's clearer.
> 
> My point is more than you're doing redundant work here. You create queue, and
> then later create a list from it. The initial queue gives you no value.

Done.

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
File
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart
(right):

https://codereview.chromium.org/331263002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformers_needed_by_transformers.dart:180:
final _imports = new Map<Uri, Set<Uri>>();
On 2014/06/18 18:24:48, Bob Nystrom wrote:
> On 2014/06/17 23:38:51, nweiz wrote:
> > On 2014/06/17 19:59:24, Bob Nystrom wrote:
> > > imports -> directives.
> > 
> > I can do that if you want, but given that neither term is strictly accurate
> > ("library" is also a directive, for example) I think "import" makes the
logic
> > surrounding this a lot clearer than "directive" does.
> 
> I do think "directive" is clearer than "import". The latter may make people
> think this actively does not work with exports and parts, which it does.
Library
> tags are technically directives too, but I don't think that would confuse
> anyone.

I disagree; I think "import" conveys the fact that these are relationships where
one file's public members become intermingled with another's, where "directive"
doesn't convey much of anything at all. Also, this *does* actively not work with
parts.

That said, I don't want to block the CL on this discussion, so done.

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:108: /// Given
[transformerDependencies], a directed acyclic xgraph, returns a list of
On 2014/06/18 18:24:48, Bob Nystrom wrote:
> Long line. Also "xgraph" -> "graph".

Done.

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart
(right):

https://codereview.chromium.org/331263002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformers_needed_by_transformers/conservative_dependencies_test.dart:52:
// Even though foo and bar they don't modify pkg/lib/transformer.dart
On 2014/06/18 18:24:48, Bob Nystrom wrote:
> Remove "they".

Done.

[nweiz, 2014-06-18 20:07:50]

Committed patchset #3 manually as r37461 (presubmit successful).