Serialize barback assets more efficiently.

Serialize barback assets more efficiently.

This adds an [InternalAsset] class to barback that supports serialization
operations, and specializes those operations for each asset type (for example,
FileAssets will just send the file path across isolates). This class isn't
exposed publicly, but pub imports it and uses it to handle asset serialization.

BUG=14430
R=rnystrom@google.com

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

[Bob Nystrom, 2013-11-06 21:29:08]

I'm super excited about this change.

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
File pkg/barback/lib/src/internal_asset.dart (right):

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:37: }
How about just moving this to Asset and getting rid of this class?

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:94: 'contents': _contents.toList()
Why copy the list? It's immutable (or, at least, it should be).

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/seria...
File pkg/barback/lib/src/serialize.dart (right):

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/seria...
pkg/barback/lib/src/serialize.dart:19: var receivePort = new ReceivePort();
Does this get closed by .first?

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

https://codereview.chromium.org/55983005/diff/40001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:12: import
'package:barback/src/internal_asset.dart';
Importing from "src"!

How about making a separate "serialization.dart" library in lib in barback?

[nweiz, 2013-11-07 00:44:48]

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
File pkg/barback/lib/src/internal_asset.dart (right):

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:37: }
On 2013/11/06 21:29:08, Bob Nystrom wrote:
> How about just moving this to Asset and getting rid of this class?

I really didn't want to commit to a publicly-visible serialization API before we
have a good solution for custom asset subclasses. Especially if we're going to
try to bring barback to 1.0 soon, I don't want us to decide to go a different
direction to support more general serialization and then have to go on supporint
[Asset.serialize] until 2.0 because someone could theoretically be using it.

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:94: 'contents': _contents.toList()
On 2013/11/06 21:29:08, Bob Nystrom wrote:
> Why copy the list? It's immutable (or, at least, it should be).

The user could pass anything that implements List<int>, but only dart:core lists
are actually serializable.

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/seria...
File pkg/barback/lib/src/serialize.dart (right):

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/seria...
pkg/barback/lib/src/serialize.dart:19: var receivePort = new ReceivePort();
On 2013/11/06 21:29:08, Bob Nystrom wrote:
> Does this get closed by .first?

Yes, thank goodness. [Stream.first] cancels its subscription after the first
message is received, and canceling a subscription on a [ReceivePort] closes the
port.

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

https://codereview.chromium.org/55983005/diff/40001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:12: import
'package:barback/src/internal_asset.dart';
On 2013/11/06 21:29:08, Bob Nystrom wrote:
> Importing from "src"!

I think it's a little gross but not unforgivable to have "friend" libraries that
access one another's src libraries, as long as each knows the other is going to
do so.

> How about making a separate "serialization.dart" library in lib in barback?

Same reason I didn't put the serialization stuff on Asset; I don't want to lock
us into this serialization API before we have time to thoroughly think through
how we want to handle custom assets.

[Bob Nystrom, 2013-11-07 18:04:56]

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
File pkg/barback/lib/src/internal_asset.dart (right):

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:37: }
On 2013/11/07 00:44:48, nweiz wrote:
> On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > How about just moving this to Asset and getting rid of this class?
> 
> I really didn't want to commit to a publicly-visible serialization API before
we
> have a good solution for custom asset subclasses.

We could put it in a separate library (lib/serialization.dart). Given that
barback is still <1.0, we aren't committed to any API. I'm definitely not
excited about committing to exposing InternalAsset and the other concrete asset
classes in the public API.

> Especially if we're going to
> try to bring barback to 1.0 soon, I don't want us to decide to go a different
> direction to support more general serialization and then have to go on
supporint
> [Asset.serialize] until 2.0 because someone could theoretically be using it.

My hunch is that we won't hit 1.0 soon. I think we'll be doing some non-trivial
performance work that will likely impact the public API and that stuff will take
a while to settle down. (Stuff we've talked about like being able to ask a
transformer if it *may* generate an asset, etc.)

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:94: 'contents': _contents.toList()
On 2013/11/07 00:44:48, nweiz wrote:
> On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > Why copy the list? It's immutable (or, at least, it should be).
> 
> The user could pass anything that implements List<int>, but only dart:core
lists
> are actually serializable.

Ah, makes sense. Leave a comment to that effect.

This may be a perf hit. Maybe add a TODO to see if we can avoid the copy in some
cases?

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

https://codereview.chromium.org/55983005/diff/40001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:12: import
'package:barback/src/internal_asset.dart';
On 2013/11/07 00:44:48, nweiz wrote:
> On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > Importing from "src"!
> 
> I think it's a little gross but not unforgivable to have "friend" libraries
that
> access one another's src libraries, as long as each knows the other is going
to
> do so.
> 
> > How about making a separate "serialization.dart" library in lib in barback?
> 
> Same reason I didn't put the serialization stuff on Asset; I don't want to
lock
> us into this serialization API before we have time to thoroughly think through
> how we want to handle custom assets.

See my previous comment. I think we can put this in lib without being locked in
since we're <1.0.

I'm definitely not keen on pub importing stuff in src. The barback tests don't
cover the APIs of libraries directly in src, so it would be possible to break
pub like this while having all of barback's tests still pass.

[nweiz, 2013-11-07 22:36:08]

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
File pkg/barback/lib/src/internal_asset.dart (right):

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:37: }
On 2013/11/07 18:04:56, Bob Nystrom wrote:
> On 2013/11/07 00:44:48, nweiz wrote:
> > On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > > How about just moving this to Asset and getting rid of this class?
> > 
> > I really didn't want to commit to a publicly-visible serialization API
before
> we
> > have a good solution for custom asset subclasses.
> 
> We could put it in a separate library (lib/serialization.dart). Given that
> barback is still <1.0, we aren't committed to any API. I'm definitely not
> excited about committing to exposing InternalAsset and the other concrete
asset
> classes in the public API.

"lib/serialization.dart" would still be part of the public API. Anything not in
"src" is part of the public API. The only way to avoid exposing these is to keep
them in "src".

> My hunch is that we won't hit 1.0 soon. I think we'll be doing some
non-trivial
> performance work that will likely impact the public API and that stuff will
take
> a while to settle down. (Stuff we've talked about like being able to ask a
> transformer if it *may* generate an asset, etc.)

Even so, we shouldn't add a public API that no external users want and that we
anticipate will change in the future.

https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
pkg/barback/lib/src/internal_asset.dart:94: 'contents': _contents.toList()
On 2013/11/07 18:04:56, Bob Nystrom wrote:
> On 2013/11/07 00:44:48, nweiz wrote:
> > On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > > Why copy the list? It's immutable (or, at least, it should be).
> > 
> > The user could pass anything that implements List<int>, but only dart:core
> lists
> > are actually serializable.
> 
> Ah, makes sense. Leave a comment to that effect.

Done.

> This may be a perf hit. Maybe add a TODO to see if we can avoid the copy in
some
> cases?

I've just gone ahead and added a [runtimeType] check.

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

https://codereview.chromium.org/55983005/diff/40001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:12: import
'package:barback/src/internal_asset.dart';
On 2013/11/07 18:04:56, Bob Nystrom wrote:
> On 2013/11/07 00:44:48, nweiz wrote:
> > On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > > Importing from "src"!
> > 
> > I think it's a little gross but not unforgivable to have "friend" libraries
> that
> > access one another's src libraries, as long as each knows the other is going
> to
> > do so.
> > 
> > > How about making a separate "serialization.dart" library in lib in
barback?
> > 
> > Same reason I didn't put the serialization stuff on Asset; I don't want to
> lock
> > us into this serialization API before we have time to thoroughly think
through
> > how we want to handle custom assets.
> 
> See my previous comment. I think we can put this in lib without being locked
in
> since we're <1.0.

The fact that we're theoretically allowed to break the API doesn't mean we
should expose functionality that we plan to break, especially if we get no value
from exposing the functionality.

> I'm definitely not keen on pub importing stuff in src. The barback tests don't
> cover the APIs of libraries directly in src, so it would be possible to break
> pub like this while having all of barback's tests still pass.

I'm not sure what you mean by this. I added a bunch of tests in asset_test that
test exactly the API that's being used by pub. Besides, we have bots that run
the pub tests on every check-in. There's no risk that we don't notice a change
in barback that breaks pub.

[Bob Nystrom, 2013-11-08 01:05:52]

On 2013/11/07 22:36:08, nweiz wrote:
>
https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
> File pkg/barback/lib/src/internal_asset.dart (right):
> 
>
https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
> pkg/barback/lib/src/internal_asset.dart:37: }
> On 2013/11/07 18:04:56, Bob Nystrom wrote:
> > On 2013/11/07 00:44:48, nweiz wrote:
> > > On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > > > How about just moving this to Asset and getting rid of this class?
> > > 
> > > I really didn't want to commit to a publicly-visible serialization API
> before
> > we
> > > have a good solution for custom asset subclasses.
> > 
> > We could put it in a separate library (lib/serialization.dart). Given that
> > barback is still <1.0, we aren't committed to any API. I'm definitely not
> > excited about committing to exposing InternalAsset and the other concrete
> asset
> > classes in the public API.
> 
> "lib/serialization.dart" would still be part of the public API. Anything not
in
> "src" is part of the public API. The only way to avoid exposing these is to
keep
> them in "src".
> 
> > My hunch is that we won't hit 1.0 soon. I think we'll be doing some
> non-trivial
> > performance work that will likely impact the public API and that stuff will
> take
> > a while to settle down. (Stuff we've talked about like being able to ask a
> > transformer if it *may* generate an asset, etc.)
> 
> Even so, we shouldn't add a public API that no external users want and that we
> anticipate will change in the future.
> 
>
https://codereview.chromium.org/55983005/diff/40001/pkg/barback/lib/src/inter...
> pkg/barback/lib/src/internal_asset.dart:94: 'contents': _contents.toList()
> On 2013/11/07 18:04:56, Bob Nystrom wrote:
> > On 2013/11/07 00:44:48, nweiz wrote:
> > > On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > > > Why copy the list? It's immutable (or, at least, it should be).
> > > 
> > > The user could pass anything that implements List<int>, but only dart:core
> > lists
> > > are actually serializable.
> > 
> > Ah, makes sense. Leave a comment to that effect.
> 
> Done.
> 
> > This may be a perf hit. Maybe add a TODO to see if we can avoid the copy in
> some
> > cases?
> 
> I've just gone ahead and added a [runtimeType] check.
> 
>
https://codereview.chromium.org/55983005/diff/40001/sdk/lib/_internal/pub/lib...
> File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):
> 
>
https://codereview.chromium.org/55983005/diff/40001/sdk/lib/_internal/pub/lib...
> sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:12: import
> 'package:barback/src/internal_asset.dart';
> On 2013/11/07 18:04:56, Bob Nystrom wrote:
> > On 2013/11/07 00:44:48, nweiz wrote:
> > > On 2013/11/06 21:29:08, Bob Nystrom wrote:
> > > > Importing from "src"!
> > > 
> > > I think it's a little gross but not unforgivable to have "friend"
libraries
> > that
> > > access one another's src libraries, as long as each knows the other is
going
> > to
> > > do so.
> > > 
> > > > How about making a separate "serialization.dart" library in lib in
> barback?
> > > 
> > > Same reason I didn't put the serialization stuff on Asset; I don't want to
> > lock
> > > us into this serialization API before we have time to thoroughly think
> through
> > > how we want to handle custom assets.
> > 
> > See my previous comment. I think we can put this in lib without being locked
> in
> > since we're <1.0.
> 
> The fact that we're theoretically allowed to break the API doesn't mean we
> should expose functionality that we plan to break, especially if we get no
value
> from exposing the functionality.
> 
> > I'm definitely not keen on pub importing stuff in src. The barback tests
don't
> > cover the APIs of libraries directly in src, so it would be possible to
break
> > pub like this while having all of barback's tests still pass.
> 
> I'm not sure what you mean by this. I added a bunch of tests in asset_test
that
> test exactly the API that's being used by pub. Besides, we have bots that run
> the pub tests on every check-in. There's no risk that we don't notice a change
> in barback that breaks pub.

Just to follow up in text:

After our discussion, I think we agreed to make top-level functions in the asset
library to serialize and deserialize and then get rid of InternalAsset? Is that
right?

- bob

[nweiz, 2013-11-08 03:01:49]

Yes; done. PTAL

[Bob Nystrom, 2013-11-08 19:22:14]

Couple of nits but LGTM. Very nice!

https://codereview.chromium.org/55983005/diff/160001/pkg/barback/lib/src/inte...
File pkg/barback/lib/src/internal_asset.dart (right):

https://codereview.chromium.org/55983005/diff/160001/pkg/barback/lib/src/inte...
pkg/barback/lib/src/internal_asset.dart:29: // so we have to convert it to a
built-in [List] first.
Move this comment up.

https://codereview.chromium.org/55983005/diff/160001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/55983005/diff/160001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:34: import
'http://<<HOST_AND_PORT>>/packages/barback/src/internal_asset.dart';
Add a TODO to not import from "src" once barback has a well-defined public API
for serialization.

[nweiz, 2013-11-08 22:03:49]

https://codereview.chromium.org/55983005/diff/160001/pkg/barback/lib/src/inte...
File pkg/barback/lib/src/internal_asset.dart (right):

https://codereview.chromium.org/55983005/diff/160001/pkg/barback/lib/src/inte...
pkg/barback/lib/src/internal_asset.dart:29: // so we have to convert it to a
built-in [List] first.
On 2013/11/08 19:22:14, Bob Nystrom wrote:
> Move this comment up.

Done.

https://codereview.chromium.org/55983005/diff/160001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/55983005/diff/160001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:34: import
'http://<<HOST_AND_PORT>>/packages/barback/src/internal_asset.dart';
On 2013/11/08 19:22:14, Bob Nystrom wrote:
> Add a TODO to not import from "src" once barback has a well-defined public API
> for serialization.

Done.

[nweiz, 2013-11-08 22:14:11]

Committed patchset #5 manually as r30126 (presubmit successful).