Load instances of the same transformer from the same isolate.

Load instances of the same transformer from the same isolate.

Previously, two transformers from the same library but with different
configuration was considered completely different from one another. This was not
only inefficient but incorrect: it meant that a transformer could be loaded with
the wrong code.

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

[Bob Nystrom, 2014-06-18 23:12:05]

Is this an optimization? What kind of performance improvement do you see? What
should we expect to see after we have import hooks?

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:91: //
TODO(nweiz): remove the [newFuture] here when issue 17305 is fixed. If
Long lines.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:240:
ExcludingTransformer.wrap(transformer, config)]);
Format like:

...([
  Excluding...
]);

or:

...(
    [Excluding...]);

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:241: // Handle
any exclusions.
Move this comment up.

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_config.dart:14: /// to that
package's use of the transformer.
Probably just remove "package's". You can have the same transformer multiple
times in one package:

transformers:
-
  - $dartj2:
      $include: foo.dart
      minify: true
  - $dartj2:
      $include: bar.dart
      minify: false

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_config.dart:28: /// Each
inclusion is an asset path. If this set is non-empty, than *only*
"than" -> "then".

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_config.dart:47: /// Returns
whether this id excludes certain asset ids from being processed.
"id" -> "config".

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_id.dart:3: // BSD-style
license that can be found in the LICENSE file.
Are there any changes in here worth noting or is this just a move?

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:29: /// A map
indicating the barback server URLs for each [TransformerId] that's
Clarify what a "barback server URL" is. An example would help.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:38: /// This
doesn't actually load the transformers, since a [TransformerId]
"load the" -> "instantiate any".

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:40: /// loaded
using [create].
"loaded" -> "constructed".

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:60:
code.writeln("import r'$baseUrl/packages/\$pub/transformer_isolate.dart';");
Long line.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:99:
Future<Set<Transformer>> create(TransformerConfig config) {
Do we ever intend to support reusing transformer instances if the configs are
identical?

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:103: //
TODO(nweiz): support non-JSON-encodable configuration maps.
Do we actually indent to support this?

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformer/loads_different_configurations_from_the_same_isolate_test.dart
(right):

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/loads_different_configurations_from_the_same_isolate_test.dart:36:
{"foo/first": {
Style like:

{
  "foo/first": {
    ...
  }
}

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/loads_different_configurations_from_the_same_isolate_test.dart:48:
]).create();
This is incredibly hard to follow. It needs some detailed docs. You have
something like a transformer that is used to transform two other transformers
with different configurations and then those subsequent transformers are used to
transform some other Dart code?

What does this have to do with testing that two transformers are in the same
isolate?

[nweiz, 2014-06-18 23:50:31]

This should improve the load time of packages that use the same transformer with
different configurations, but we don't have any packages that do that handy so I
haven't benchmarked. More importantly, it fixes the bug described in issue 19261
and tested in the new test.

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:91: //
TODO(nweiz): remove the [newFuture] here when issue 17305 is fixed. If
On 2014/06/18 23:12:03, Bob Nystrom wrote:
> Long lines.

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:240:
ExcludingTransformer.wrap(transformer, config)]);
On 2014/06/18 23:12:03, Bob Nystrom wrote:
> Format like:
> 
> ...([
>   Excluding...
> ]);
> 
> or:
> 
> ...(
>     [Excluding...]);

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:241: // Handle
any exclusions.
On 2014/06/18 23:12:03, Bob Nystrom wrote:
> Move this comment up.

Done.

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_config.dart:14: /// to that
package's use of the transformer.
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> Probably just remove "package's". You can have the same transformer multiple
> times in one package:
> 
> transformers:
> -
>   - $dartj2:
>       $include: foo.dart
>       minify: true
>   - $dartj2:
>       $include: bar.dart
>       minify: false

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_config.dart:28: /// Each
inclusion is an asset path. If this set is non-empty, than *only*
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> "than" -> "then".

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_config.dart:47: /// Returns
whether this id excludes certain asset ids from being processed.
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> "id" -> "config".

Done.

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_id.dart:3: // BSD-style
license that can be found in the LICENSE file.
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> Are there any changes in here worth noting or is this just a move?

Just a move and a few members removed to add to TransformerConfig.

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:29: /// A map
indicating the barback server URLs for each [TransformerId] that's
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> Clarify what a "barback server URL" is. An example would help.

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:38: /// This
doesn't actually load the transformers, since a [TransformerId]
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> "load the" -> "instantiate any".

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:40: /// loaded
using [create].
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> "loaded" -> "constructed".

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:60:
code.writeln("import r'$baseUrl/packages/\$pub/transformer_isolate.dart';");
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:99:
Future<Set<Transformer>> create(TransformerConfig config) {
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> Do we ever intend to support reusing transformer instances if the configs are
> identical?

I have no particular opinion on the matter.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/transformer_isolate.dart:103: //
TODO(nweiz): support non-JSON-encodable configuration maps.
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> Do we actually indent to support this?

        I'm not sure what this TODO was referring to. Removed.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformer/loads_different_configurations_from_the_same_isolate_test.dart
(right):

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/loads_different_configurations_from_the_same_isolate_test.dart:36:
{"foo/first": {
On 2014/06/18 23:12:05, Bob Nystrom wrote:
> Style like:
> 
> {
>   "foo/first": {
>     ...
>   }
> }

Done.

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/loads_different_configurations_from_the_same_isolate_test.dart:48:
]).create();
On 2014/06/18 23:12:04, Bob Nystrom wrote:
> This is incredibly hard to follow. It needs some detailed docs. You have
> something like a transformer that is used to transform two other transformers
> with different configurations and then those subsequent transformers are used
to
> transform some other Dart code?
> 
> What does this have to do with testing that two transformers are in the same
> isolate?

Done.

[Bob Nystrom, 2014-06-19 16:49:42]

One nit, but LGTM!

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:91: //
TODO(nweiz): remove the [newFuture] here when issue 17305 is fixed. If
On 2014/06/18 23:50:30, nweiz wrote:
> On 2014/06/18 23:12:03, Bob Nystrom wrote:
> > Long lines.
> 
> Done.

Still looks long to me. Did you upload it?

[nweiz, 2014-06-19 19:34:45]

Committed patchset #4 manually as r37511 (presubmit successful).

[nweiz, 2014-06-19 19:36:43]

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

https://codereview.chromium.org/344673002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:91: //
TODO(nweiz): remove the [newFuture] here when issue 17305 is fixed. If
On 2014/06/19 16:49:42, Bob Nystrom wrote:
> On 2014/06/18 23:50:30, nweiz wrote:
> > On 2014/06/18 23:12:03, Bob Nystrom wrote:
> > > Long lines.
> > 
> > Done.
> 
> Still looks long to me. Did you upload it?

Oops, done for real.