Move barback to a more event-based model.

Move barback to a more event-based model.

The goal of this change is to make it easy to synchronously propagate the dirty
state of an asset node to all of the nodes and transforms that depend on it,
while still asynchronously processing those transforms.

In addition, this refactor ends up addressing two TODOs and one bug:

* If a node has been compiled previously and an unrelated part of its cascade is
  dirtied, it will now be returned immediately rather than waiting for the
  entire cascade to finish processing.

* If a node is consumed as the input of a transform, it will no longer be
  returned by [getAssetById].

* If the contents of a transform's primary input changes such that it's no
  longer a valid primary input for that transform, the transform will be
  removed.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-07-31 20:05:41]

Both barback before your patch and with it still intuitively feels more complex
than it should be for the problem its solving. Maybe it's just the pain of
asynchrony, or maybe there's a cleaner approach that isn't obvious to us yet.

Regardless, this patch is a big step in the right direction, especially the new
tests.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
File pkg/barback/lib/src/asset_cascade.dart (right):

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:43: /// cascade's package's source
assets, indexed by id.
Remove "indexed by id".

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:48: /// These futures are cancelable so
that if an asset is updated after a load
This implies that a PackageProvider's implementation of getAsset() is
re-entrant: it may be called before the future from a previous call for the same
ID has completed. We should document that on PackageProvider.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:170: sources.forEach((id) {
Nit, but since this is imperative code, I think a normal for loop would be more
idiomatic here.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:32: Asset get asset => _asset;
Is it an error to access this when not available? Maybe throw instead of
returning null?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:46: new
StreamController<AssetState>.broadcast(sync: true);
Indent +2.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:65: /// If the asset is already removed when
this is called, it will complete
"will complete" -> "completes"

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:93: subscription.cancel();
Move this above the if statement.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:143: /// If a new asset is created with the
same id, it should have a new node.
"should have" -> "will get"

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:158: assert(node._state !=
AssetState.AVAILABLE);
assert(node._state == AssetState.DIRTY);

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:168: /// The state of having an asset
available.
This sentence is strange. Just ditch it and use the next sentence as the
summary.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:170: /// This state indicates that the node
has a concrete asset loaded, available,
Remove "This state indicates that".

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:173: /// [AssetState.DIRTY].
This reads weird. Are you saying that an asset in the AVAILABLE state *cannot*
be marked available again? If so, I would just say, "An asset can only be marked
available again from the DIRTY state."

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:179: /// good. A removed asset will never
enter another state.
Same as above. Remove first sentence, use this one here, and ditch the "This
state indicates that".

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:185: /// removed), but the concrete asset is
not yet available.
Ditto.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:186: static final DIRTY = new
AssetState._("dirty");
Make these all constants instead of final, or name them lowercase.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:199: AssetState._(this.name);
Make this a const constructor.

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

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:64: var _adjustTransformersFutures = new
Map<AssetId, Future>();
"adjust" is a bit confusing to me. How about "possible"?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:102: if
(_adjustTransformersFutures.containsKey(node.id)) {
Switch the order of cases here and do if (!_adjusted...) then return early from
there so you don't have to indent everything.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:107: // of that asset is.
This comment is confused compared to the code. Is the idea that we are *already*
waiting (i.e. there is a future for this node ID in _adjustTransformersFutures)
so we don't have to wait again and can return here?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:143: if (!inputs.containsKey(id)) return null;
How about some blank lines above the comments in here?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:144: // If the asset has inputs but no
_transforms set, that means that
means what?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:145: if (!_transforms.containsKey(id)) return
null;
I don't understand this. If there's no transformer for the id, doesn't that mean
that it *is* potentially available?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:163: void _adjustTransformers(AssetNode node) {
This method name isn't very clear. How about _findNewTransforms or something
like that?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:176: node.onStateChange.first.then((state) {
Do we need to handle the .first future having an error?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:213: // Add new transforms for transformers that
consider [asset] to be a primary
"[asset]" -> "[node]'s asset"

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:219: Future _addNewTransforms(AssetNode node,
Set<Transformer> oldTransformers) {
"new" -> "fresh" to correspond with "stale" above? Our terminology is a bit all
over the place. It would be nice to have a consistent set of metaphors here,
though it's tricky.

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

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:128: if (newInput.id == primary.id)
continue;
How can this case get hit?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:142: // Any ids that are for the wrong
package are invalid.
"for the wrong" -> "to a different"

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:155: for (var id in
_outputControllers.keys.toList()) {
If we make AssetSet implement Set, you could do:

.toList() -> .toSet().difference(newOutputs)

here and lose the continue line.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:163: var controller =
_outputControllers[asset.id];
Use putIfAbsent() here?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/transform_test.dart (right):

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:317: test("applies a
transform when it becomes newly primary", () {
Nice.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:346: schedule(() =>
removeSources(["app|foo.txt"]));
What happens if you don't remove first and just modify it in place?

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:572: // Make sure we're
waiting on isPrimary
"." here and elsewhere.

[nweiz, 2013-07-31 22:47:53]

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
File pkg/barback/lib/src/asset_cascade.dart (right):

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:43: /// cascade's package's source
assets, indexed by id.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Remove "indexed by id".

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:48: /// These futures are cancelable so
that if an asset is updated after a load
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> This implies that a PackageProvider's implementation of getAsset() is
> re-entrant: it may be called before the future from a previous call for the
same
> ID has completed. We should document that on PackageProvider.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:170: sources.forEach((id) {
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Nit, but since this is imperative code, I think a normal for loop would be
more
> idiomatic here.

Done. Not sure why I wrote [forEach]. Too much Ruby, I guess.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:32: Asset get asset => _asset;
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Is it an error to access this when not available? Maybe throw instead of
> returning null?

I think it's reasonable to access it and check for null. I think of it as the
same as Map -- you can use [containsKey], but [[]] still doesn't throw for a
non-existent key.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:46: new
StreamController<AssetState>.broadcast(sync: true);
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Indent +2.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:65: /// If the asset is already removed when
this is called, it will complete
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "will complete" -> "completes"

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:93: subscription.cancel();
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Move this above the if statement.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:143: /// If a new asset is created with the
same id, it should have a new node.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "should have" -> "will get"

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:158: assert(node._state !=
AssetState.AVAILABLE);
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> assert(node._state == AssetState.DIRTY);

I intentionally avoided that because I'm planning on adding an ERROR state in
the future that will also be valid here.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:168: /// The state of having an asset
available.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> This sentence is strange. Just ditch it and use the next sentence as the
> summary.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:170: /// This state indicates that the node
has a concrete asset loaded, available,
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Remove "This state indicates that".

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:173: /// [AssetState.DIRTY].
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> This reads weird. Are you saying that an asset in the AVAILABLE state *cannot*
> be marked available again? If so, I would just say, "An asset can only be
marked
> available again from the DIRTY state."

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:179: /// good. A removed asset will never
enter another state.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Same as above. Remove first sentence, use this one here, and ditch the "This
> state indicates that".

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:185: /// removed), but the concrete asset is
not yet available.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:186: static final DIRTY = new
AssetState._("dirty");
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Make these all constants instead of final, or name them lowercase.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:199: AssetState._(this.name);
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Make this a const constructor.

Done.

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

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:64: var _adjustTransformersFutures = new
Map<AssetId, Future>();
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "adjust" is a bit confusing to me. How about "possible"?

The name comes from the fact that these are futures running
[_adjustTransformers]. I feel like "possible" implies that these are futures
that may return transformers, which is incorrect; they don't return anything.
All that matters is their side effects and when they end.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:102: if
(_adjustTransformersFutures.containsKey(node.id)) {
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Switch the order of cases here and do if (!_adjusted...) then return early
from
> there so you don't have to indent everything.

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:107: // of that asset is.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> This comment is confused compared to the code. Is the idea that we are
*already*
> waiting (i.e. there is a future for this node ID in
_adjustTransformersFutures)
> so we don't have to wait again and can return here?

The comment is notionally attached to the _pendingNewInputs line. I'll
reorganize to clarify.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:143: if (!inputs.containsKey(id)) return null;
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> How about some blank lines above the comments in here?

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:144: // If the asset has inputs but no
_transforms set, that means that
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> means what?

This can't actually happen any more; removed the clause.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:145: if (!_transforms.containsKey(id)) return
null;
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> I don't understand this. If there's no transformer for the id, doesn't that
mean
> that it *is* potentially available?

There's a difference between no transformer being available and the set of
transforms itself being null. But as I mentioned above, that can never happen
when the input exists, so I've removed this clause.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:163: void _adjustTransformers(AssetNode node) {
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> This method name isn't very clear. How about _findNewTransforms or something
> like that?

It doesn't just find new transforms. It also checks whether existing transforms
still apply to modified inputs. The best way to think about it is that it makes
sure that all the transforms for a given input are wired up correctly, whatever
that takes. I think "_adjustTransforms" is as good a name as any for that.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:176: node.onStateChange.first.then((state) {
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Do we need to handle the .first future having an error?

Done. It now pipes the error so that it will be emitted via [process].

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:213: // Add new transforms for transformers that
consider [asset] to be a primary
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "[asset]" -> "[node]'s asset"

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:213: // Add new transforms for transformers that
consider [asset] to be a primary
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "[asset]" -> "[node]'s asset"

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:219: Future _addNewTransforms(AssetNode node,
Set<Transformer> oldTransformers) {
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "new" -> "fresh" to correspond with "stale" above? Our terminology is a bit
all
> over the place. It would be nice to have a consistent set of metaphors here,
> though it's tricky.

Done.

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

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:128: if (newInput.id == primary.id)
continue;
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> How can this case get hit?

This case is hit whenever the transform loads the primary input.
[Transform.primaryInput] forwards to [Transform.getInput], which adds the input
to [newInput].

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:142: // Any ids that are for the wrong
package are invalid.
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "for the wrong" -> "to a different"

Done.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:155: for (var id in
_outputControllers.keys.toList()) {
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> If we make AssetSet implement Set, you could do:
> 
> .toList() -> .toSet().difference(newOutputs)
> 
> here and lose the continue line.

Eh, I think this is clear enough.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:163: var controller =
_outputControllers[asset.id];
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> Use putIfAbsent() here?

That doesn't work well when we want to call [setAvailable] only if it was
already in the map, not if it was newly created.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/transform_test.dart (right):

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:346: schedule(() =>
removeSources(["app|foo.txt"]));
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> What happens if you don't remove first and just modify it in place?

Added test.

https://codereview.chromium.org/21275003/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:572: // Make sure we're
waiting on isPrimary
On 2013/07/31 20:05:41, Bob Nystrom wrote:
> "." here and elsewhere.

Done.

[Bob Nystrom, 2013-07-31 23:20:15]

LGTM!

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

https://codereview.chromium.org/21275003/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_node.dart:163: var controller =
_outputControllers[asset.id];
On 2013/07/31 22:47:53, nweiz wrote:
> On 2013/07/31 20:05:41, Bob Nystrom wrote:
> > Use putIfAbsent() here?
> 
> That doesn't work well when we want to call [setAvailable] only if it was
> already in the map, not if it was newly created.

You could just call that from within the pubIfAbsent() callback itself. But do
whichever you prefer.

[nweiz, 2013-07-31 23:51:46]

Committed patchset #2 manually as r25693 (presubmit successful).