Cache snapshots of (mostly) immutable transformer phases.

Cache snapshots of (mostly) immutable transformer phases.

This shaves off a pretty significant amount of time for packages that are
transformed but whose transformers never change (I saw a ~450ms gain in one of
my tests).

R=rnystrom@google.com
BUG=20859
Committed: https://code.google.com/p/dart/source/detail?r=40392

[Bob Nystrom, 2014-09-10 17:58:11]

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/asset/...
File sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart (right):

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/asset/...
sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart:15: /// The mirror
system.
This comment doesn't add much. Are you caching this for perf? If so, maybe say
that.

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:23: /// should be
called called for each phase being loaded, followed by [save] once
called called

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:35: /// The
transformers in the previously cached phase snapshots.
Document this in a bit more detail:

- each list entry is a phase, items are in phase order
- each phase is the set of transformers in that phase

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:38: /// The
transformers in the newly-cached or reused phase snapshots.
Here too. In particular, how indices map to phase numbers.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:55: // being used.
When is this not the case? Globally activating a path package? Might be good to
mention at least one example here.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:59:
environment.rootPackage.dir, ".pub", "transformers",
Do we have a guideline on joining multiple literal path components? Both
p.join("a", "b") and p.join("a/b") are valid on all platforms. Is there a reason
to prefer the former?

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:60:
Uri.encodeComponent(environment.mode.name));
What's going to happen here when we add other stuff to the environment like
console/browser?

Other transformer configs can also affect this, can't they? I'm thinking
transformer A uses some random config to decide how to transformer a .dart file
which is then imported by transformer B. Does the cache take that into account?

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:75: ///
[phaseNumber] is the number of [phase] in the transformer load order.
0- or 1-based?

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:80: var package =
_graph.lockFile.packages[id.package];
Is for handling the root package? If so, document.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:83: return source
is! CachedSource;
What if the package depends on something mutable? You can have a git package
that references a path one. :(

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:95: if
(_oldSnapshotPhases.length >= phaseNumber &&
Document the underlying logic here.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:116:
}).join("\n"));
This is very terse, but it makes it hard to envision what the output looks like.
Maybe document it or unpack this to some more imperative + StringBuffery.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:120: i <
_oldSnapshotPhases.length + 1; i++) {
Nit, but I'd use <= instead < ... + 1.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:144: // transformer
ids in each snapshotted phase. We can re-use phases with
This will do bad things if we ever put a comma in TransformerId.toString().

In general, I'm hesitant to rely on .toString() for critical application
behavior. I think programmers tend to assume .toString() is just for debugging
or user display and can be changed pretty freely.

It might be worth making a separate getter on TransformerId specifically to
serialize it.

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_loader.dart:51: /// This will
skip any ids that have already been loaded.
"will skip" -> "skips".

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_loader.dart:59:
}).then((isolate) {
await?

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_loader.dart:71:
Future<Set<Transformer>> transformersFor(TransformerConfig config) {
This can definitely benefit from await.

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:178: }).then((_) {
await?

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:189: result.stderr.join("\n"));
I think this fits on the previous line.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/transformer/cache_test.dart (right):

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:70: 
Document running again to make sure running the snapshot works.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:100:
process.shouldExit();
I don't think it's worth re-running here.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:159:
solo_integration("removes an unused phase snapshot", () {
Oopsie!

[nweiz, 2014-09-10 23:30:05]

Code review changes

[nweiz, 2014-09-10 23:30:13]

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/asset/...
File sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart (right):

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/asset/...
sdk/lib/_internal/pub/asset/dart/transformer_isolate.dart:15: /// The mirror
system.
On 2014/09/10 17:58:08, Bob Nystrom wrote:
> This comment doesn't add much. Are you caching this for perf? If so, maybe say
> that.

Done.

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:23: /// should be
called called for each phase being loaded, followed by [save] once
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> called called

I'm tempted to replace this with "called called called" and see if you notice.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:35: /// The
transformers in the previously cached phase snapshots.
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> Document this in a bit more detail:
> 
> - each list entry is a phase, items are in phase order
> - each phase is the set of transformers in that phase

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:38: /// The
transformers in the newly-cached or reused phase snapshots.
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> Here too. In particular, how indices map to phase numbers.

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:55: // being used.
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> When is this not the case? Globally activating a path package? Might be good
to
> mention at least one example here.

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:59:
environment.rootPackage.dir, ".pub", "transformers",
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> Do we have a guideline on joining multiple literal path components? Both
> p.join("a", "b") and p.join("a/b") are valid on all platforms. Is there a
reason
> to prefer the former?

I guess not. Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:60:
Uri.encodeComponent(environment.mode.name));
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> What's going to happen here when we add other stuff to the environment like
> console/browser?
> 
> Other transformer configs can also affect this, can't they? I'm thinking
> transformer A uses some random config to decide how to transformer a .dart
file
> which is then imported by transformer B. Does the cache take that into
account?

As we discussed in-person, I'm going to change this to only snapshot the first
phase, which will avoid the issue.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:75: ///
[phaseNumber] is the number of [phase] in the transformer load order.
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> 0- or 1-based?

N/A

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:80: var package =
_graph.lockFile.packages[id.package];
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> Is for handling the root package? If so, document.

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:83: return source
is! CachedSource;
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> What if the package depends on something mutable? You can have a git package
> that references a path one. :(

Added a TODO.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:95: if
(_oldSnapshotPhases.length >= phaseNumber &&
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> Document the underlying logic here.

N/A

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:116:
}).join("\n"));
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> This is very terse, but it makes it hard to envision what the output looks
like.
> Maybe document it or unpack this to some more imperative + StringBuffery.

N/A

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:120: i <
_oldSnapshotPhases.length + 1; i++) {
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> Nit, but I'd use <= instead < ... + 1.

N/A

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:144: // transformer
ids in each snapshotted phase. We can re-use phases with
On 2014/09/10 17:58:09, Bob Nystrom wrote:
> This will do bad things if we ever put a comma in TransformerId.toString().
> 
> In general, I'm hesitant to rely on .toString() for critical application
> behavior. I think programmers tend to assume .toString() is just for debugging
> or user display and can be changed pretty freely.
> 
> It might be worth making a separate getter on TransformerId specifically to
> serialize it.

Done.

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_loader.dart:51: /// This will
skip any ids that have already been loaded.
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> "will skip" -> "skips".

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_loader.dart:59:
}).then((isolate) {
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> await?

I didn't change this because this is just copied from load_all_transformers, but
sure.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_loader.dart:71:
Future<Set<Transformer>> transformersFor(TransformerConfig config) {
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> This can definitely benefit from await.

Done.

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:178: }).then((_) {
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> await?

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:189: result.stderr.join("\n"));
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> I think this fits on the previous line.

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/transformer/cache_test.dart (right):

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:70: 
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> Document running again to make sure running the snapshot works.

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:100:
process.shouldExit();
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> I don't think it's worth re-running here.

Done.

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:159:
solo_integration("removes an unused phase snapshot", () {
On 2014/09/10 17:58:10, Bob Nystrom wrote:
> Oopsie!

Done.

[Bob Nystrom, 2014-09-12 21:17:18]

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:83: return source
is! CachedSource;
On 2014/09/10 23:30:11, nweiz wrote:
> On 2014/09/10 17:58:09, Bob Nystrom wrote:
> > What if the package depends on something mutable? You can have a git package
> > that references a path one. :(
> 
> Added a TODO.

Is this particularly hard to do? I think we really want to handle this
correctly. Right now, if the user runs into this case, they'll have stale code
and little ability to do anything about it short of blowing away this snapshot,
right?

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

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:67: var
snapshotPath = cache == null || !first ? null :
Document why we're only doing this for the first phase.

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart (right):

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:21: /// This uses
the [_phaseTransformers] notion of a phase. Transformers are
Maybe we need a different term here. "stage"?

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:27: /// independent
of any configuration.
I'd add that since it's pretty rare for transformers to import transformed code,
most packages only have a single phase.

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/transformer/cache_test.dart (right):

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:141:
integration("recaches if the transformer version changes", () {
Also test that it recaches if a transitive dependency changes.

[nweiz, 2014-09-16 00:10:13]

Code review changes

[nweiz, 2014-09-16 00:10:53]

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:83: return source
is! CachedSource;
On 2014/09/12 21:17:17, Bob Nystrom wrote:
> On 2014/09/10 23:30:11, nweiz wrote:
> > On 2014/09/10 17:58:09, Bob Nystrom wrote:
> > > What if the package depends on something mutable? You can have a git
package
> > > that references a path one. :(
> > 
> > Added a TODO.
> 
> Is this particularly hard to do? I think we really want to handle this
> correctly. Right now, if the user runs into this case, they'll have stale code
> and little ability to do anything about it short of blowing away this
snapshot,
> right?

I don't think there are a lot of git packages out there that have path
dependencies. That's fundamentally a pretty weird thing to do, since git repos
are usually used to share code.

Regardless, I do have a fix for this in a future CL.

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

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:67: var
snapshotPath = cache == null || !first ? null :
On 2014/09/12 21:17:17, Bob Nystrom wrote:
> Document why we're only doing this for the first phase.

Done.

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart (right):

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:21: /// This uses
the [_phaseTransformers] notion of a phase. Transformers are
On 2014/09/12 21:17:17, Bob Nystrom wrote:
> Maybe we need a different term here. "stage"?

Done.

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:27: /// independent
of any configuration.
On 2014/09/12 21:17:17, Bob Nystrom wrote:
> I'd add that since it's pretty rare for transformers to import transformed
code,
> most packages only have a single phase.

Done.

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/transformer/cache_test.dart (right):

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:141:
integration("recaches if the transformer version changes", () {
On 2014/09/12 21:17:17, Bob Nystrom wrote:
> Also test that it recaches if a transitive dependency changes.

Done.

https://codereview.chromium.org/559833004/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:141:
integration("recaches if the transformer version changes", () {
On 2014/09/12 21:17:17, Bob Nystrom wrote:
> Also test that it recaches if a transitive dependency changes.

Done.

[Bob Nystrom, 2014-09-16 19:54:27]

lgtm

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

https://codereview.chromium.org/559833004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart:83: return source
is! CachedSource;
On 2014/09/16 00:10:53, nweiz wrote:
> On 2014/09/12 21:17:17, Bob Nystrom wrote:
> > On 2014/09/10 23:30:11, nweiz wrote:
> > > On 2014/09/10 17:58:09, Bob Nystrom wrote:
> > > > What if the package depends on something mutable? You can have a git
> package
> > > > that references a path one. :(
> > > 
> > > Added a TODO.
> > 
> > Is this particularly hard to do? I think we really want to handle this
> > correctly. Right now, if the user runs into this case, they'll have stale
code
> > and little ability to do anything about it short of blowing away this
> snapshot,
> > right?
> 
> I don't think there are a lot of git packages out there that have path
> dependencies. That's fundamentally a pretty weird thing to do, since git repos
> are usually used to share code.

It's not that weird if you have a single multi-package git repo that contains
subpackages and uses (relative) path deps to find them.

> Regardless, I do have a fix for this in a future CL.

SGTM.

https://codereview.chromium.org/559833004/diff/60001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/transformer/cache_test.dart (right):

https://codereview.chromium.org/559833004/diff/60001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/cache_test.dart:200: import 'dart:async';
Do you mind pulling this string out to a separate constant? It makes the package
serving code a bit hard to parse like this.

[nweiz, 2014-09-17 19:51:50]

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