Only run at most 10 transformers at once in barback.

Only run at most 10 transformers at once in barback.

BUG=14320
R=alanknight@google.com

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

[Alan Knight, 2013-10-23 00:24:06]

lgtm

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/pool.dart
File pkg/barback/lib/src/pool.dart (right):

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/pool.dart...
pkg/barback/lib/src/pool.dart:105: /// A [PoolResource] is a token that
indicates that a resource is allocated. When
80 chars

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:129: return
phase.cascade.graph.transformerPool.checkOut().then((item) {
That seems like a rather awkward way to get to the pool

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:130: return
transformer.apply(transform).whenComplete(item.release);
Is it worth defining a pool API that automatically acquires and releases. e.g.
   transformerPool.withResource( (resource) => transformer.apply(transform));
?

Also, is it right that in this expression we have a transformerPool, but we
don't obtain the transformer, we already have it, and we don't use item other
than implicitly. It might make sense if we're just limiting the number running
at once, but just wanted to be sure.

[nweiz, 2013-10-23 04:11:25]

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/pool.dart
File pkg/barback/lib/src/pool.dart (right):

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/pool.dart...
pkg/barback/lib/src/pool.dart:105: /// A [PoolResource] is a token that
indicates that a resource is allocated. When
On 2013/10/23 00:24:06, Alan Knight wrote:
> 80 chars

Done.

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/pool.dart...
pkg/barback/lib/src/pool.dart:105: /// A [PoolResource] is a token that
indicates that a resource is allocated. When
On 2013/10/23 00:24:06, Alan Knight wrote:
> 80 chars

Done.

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:129: return
phase.cascade.graph.transformerPool.checkOut().then((item) {
On 2013/10/23 00:24:06, Alan Knight wrote:
> That seems like a rather awkward way to get to the pool

I agree. I'm planning a refactor that will make the "vertical" dependencies
(e.g. PackageGraph <-> AssetCascade <-> Phase <-> TransformNode) in barback more
clear.

https://codereview.chromium.org/36213002/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:130: return
transformer.apply(transform).whenComplete(item.release);
On 2013/10/23 00:24:06, Alan Knight wrote:
> Is it worth defining a pool API that automatically acquires and releases. e.g.
>    transformerPool.withResource( (resource) => transformer.apply(transform));
> ?

Done.

> Also, is it right that in this expression we have a transformerPool, but we
> don't obtain the transformer, we already have it, and we don't use item other
> than implicitly. It might make sense if we're just limiting the number running
> at once, but just wanted to be sure.

Yeah; really what the pool is controlling is calling [apply]. I'm going to
rename it to [transformPool] to make that a bit clearer.

[nweiz, 2013-10-23 04:13:49]

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

[Bob Nystrom, 2013-10-28 17:32:21]

A couple of nits, but LGTM.

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/lib/src/pool....
File pkg/barback/lib/src/pool.dart (right):

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/lib/src/pool....
pkg/barback/lib/src/pool.dart:52: Future<PoolResource> checkOut() {
What do you think of "acquire" for this? "checkOut" seems a little strange when
the opposite operation is "release" to me. I would expect "return" or something
like that in that case.

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/lib/src/pool....
pkg/barback/lib/src/pool.dart:89: /// Indicates that some external action has
occurred and the timer should be
"Indicates that some external action" -> "A resource has been checked out or
released"

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/test/transfor...
File pkg/barback/test/transformer/mock.dart (right):

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/test/transfor...
pkg/barback/test/transformer/mock.dart:30: Future<int> get maxParallelRuns =>
schedule(() => _maxParallelRuns);
Doc comment.

[nweiz, 2013-10-29 00:06:21]

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/lib/src/pool....
File pkg/barback/lib/src/pool.dart (right):

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/lib/src/pool....
pkg/barback/lib/src/pool.dart:52: Future<PoolResource> checkOut() {
On 2013/10/28 17:32:21, Bob Nystrom wrote:
> What do you think of "acquire" for this? "checkOut" seems a little strange
when
> the opposite operation is "release" to me. I would expect "return" or
something
> like that in that case.

Done. Referring to the "acquired resources" as the set of resources that are in
use seemed weird, so I started using "allocated" for that.

https://codereview.chromium.org/36213002/diff/90001/pkg/barback/lib/src/pool....
pkg/barback/lib/src/pool.dart:89: /// Indicates that some external action has
occurred and the timer should be
On 2013/10/28 17:32:21, Bob Nystrom wrote:
> "Indicates that some external action" -> "A resource has been checked out or
> released"

Done.