Cache snapshots of (mostly) immutable transformer phases.

sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart

Index: sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart
diff --git a/sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart b/sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart
new file mode 100644
index 0000000000000000000000000000000000000000..802c77494687a12111f1cb396a813b7a8c21051f
--- /dev/null
+++ b/sdk/lib/_internal/pub/lib/src/barback/transformer_cache.dart
@@ -0,0 +1,156 @@
+// Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library pub.barback.transformer_cache;
+
+import 'package:path/path.dart' as p;
+
+import '../io.dart';
+import '../log.dart' as log;
+import '../package_graph.dart';
+import '../sdk.dart' as sdk;
+import '../source/cached.dart';
+import '../utils.dart';
+import 'asset_environment.dart';
+import 'transformer_id.dart';
+
+/// A cache of transformer snapshots.
+///
+/// Due to the phased nature of transformer loading, a snapshot can only be
+/// safely made during the load process. As such, [TransformerCache] expects to
+/// have its methods called in a certain order. In particular, [snapshotPath]
+/// should be called called for each phase being loaded, followed by [save] once

[Bob Nystrom, 2014/09/10 17:58:09]

called called

[nweiz, 2014/09/10 23:30:12]

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

+/// all the snapshots have been saved.
+class TransformerCache {
+  final PackageGraph _graph;
+
+  /// Whether further snapshots should be saved.
+  ///
+  /// If any phase contains a transformer from a mutable package, neither that
+  /// phase nor any future one can be snapshotted, since the text of the
+  /// transformers may be determined by mutable code.
+  bool _saveSnapshots;
+
+  /// The transformers in the previously cached phase snapshots.

[Bob Nystrom, 2014/09/10 17:58:09]

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

[nweiz, 2014/09/10 23:30:12]

Done.

+  List<Set<TransformerId>> _oldSnapshotPhases;
+
+  /// The transformers in the newly-cached or reused phase snapshots.

[Bob Nystrom, 2014/09/10 17:58:09]

Here too. In particular, how indices map to phase numbers.

[nweiz, 2014/09/10 23:30:11]

Done.

+  final _newSnapshotPhases = new List<Set<TransformerId>>();
+
+  /// The directory in which transformers are cached.
+  ///
+  /// This may be `null` if there's no physical entrypoint directory.
+  String _dir;
+
+  /// The directory of the manifest listing which transformers were cached.
+  String get _manifestPath => p.join(_dir, "manifest.txt");
+
+  /// Loads the transformer cache for [environment].
+  ///
+  /// This may modify the cache.
+  TransformerCache.load(AssetEnvironment environment)
+      : _graph = environment.graph,
+        // Only save compiled snapshots when a physical entrypoint package is
+        // being used.

[Bob Nystrom, 2014/09/10 17:58:10]

When is this not the case? Globally activating a path package? Might be good to
mention at least one example here.

[nweiz, 2014/09/10 23:30:12]

Done.

+        _saveSnapshots = environment.rootPackage.dir != null {
+    if (_saveSnapshots) {
+      _dir = p.join(
+          environment.rootPackage.dir, ".pub", "transformers",

[Bob Nystrom, 2014/09/10 17:58:09]

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?

[nweiz, 2014/09/10 23:30:12]

I guess not. Done.

+          Uri.encodeComponent(environment.mode.name));

[Bob Nystrom, 2014/09/10 17:58:09]

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?

[nweiz, 2014/09/10 23:30:12]

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

+      _oldSnapshotPhases = _parseManifest();
+    } else {
+      _oldSnapshotPhases = const [];
+      _dir = null;
+    }
+  }
+
+  /// Returns the path for the transformer snapshot for [phase], or `null` if
+  /// that phase shouldn't be cached.
+  ///
+  /// There may or may not exist a file at the returned path. If one does exist,
+  /// it can safely be used to load the phase. Otherwise, a snapshot of the
+  /// phase should be written there.
+  ///
+  /// [phaseNumber] is the number of [phase] in the transformer load order.

[Bob Nystrom, 2014/09/10 17:58:09]

0- or 1-based?

[nweiz, 2014/09/10 23:30:12]

N/A

+  String snapshotPath(Set<TransformerId> phase, int phaseNumber) {
+    if (!_saveSnapshots) return null;
+
+    var usesMutableTransformer = phase.any((id) {
+      var package = _graph.lockFile.packages[id.package];

[Bob Nystrom, 2014/09/10 17:58:09]

Is for handling the root package? If so, document.

[nweiz, 2014/09/10 23:30:11]

Done.

+      if (package == null) return true;
+      var source = _graph.entrypoint.cache.sources[package.source];
+      return source is! CachedSource;

[Bob Nystrom, 2014/09/10 17:58:09]

What if the package depends on something mutable? You can have a git package
that references a path one. :(

[nweiz, 2014/09/10 23:30:11]

Added a TODO.

[Bob Nystrom, 2014/09/12 21:17:17]

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?

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

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.

[Bob Nystrom, 2014/09/16 19:54:26]

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.
SGTM.

+    });
+
+    var path = p.join(_dir, "phase$phaseNumber.snapshot");
+    if (usesMutableTransformer) {
+      log.fine("Phase $phaseNumber contains mutable transformers, not "
+          "caching.");
+      _saveSnapshots = false;
+      deleteEntry(path);
+      return null;
+    }
+
+    if (_oldSnapshotPhases.length >= phaseNumber &&

[Bob Nystrom, 2014/09/10 17:58:09]

Document the underlying logic here.

[nweiz, 2014/09/10 23:30:12]

N/A

+        !_oldSnapshotPhases[phaseNumber - 1].containsAll(phase)) {
+      log.fine("Cached phase $phaseNumber is out-of-date, deleting.");
+      deleteEntry(path);
+    }
+    _newSnapshotPhases.add(phase);
+    return path;
+  }
+
+  /// Saves the manifest to the transformer cache.
+  void save() {
+    // If we didn't write any snapshots, there's no need to write a manifest.
+    if (_newSnapshotPhases.isEmpty) {
+      if (_dir != null) deleteEntry(_dir);
+      return;
+    }
+
+    ensureDir(_dir);
+    writeTextFile(_manifestPath,
+        "${sdk.version}\n" + _newSnapshotPhases.map((phase) {
+      return ordered(phase.map((id) => id.toString())).join(",");
+    }).join("\n"));

[Bob Nystrom, 2014/09/10 17:58:09]

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.

[nweiz, 2014/09/10 23:30:12]

N/A

+
+    // Delete old unused snapshots.
+    for (var i = _newSnapshotPhases.length + 1;
+         i < _oldSnapshotPhases.length + 1; i++) {

[Bob Nystrom, 2014/09/10 17:58:10]

Nit, but I'd use <= instead < ... + 1.

[nweiz, 2014/09/10 23:30:12]

N/A

+      deleteEntry(p.join(_dir, "phase$i.snapshot"));
+    }
+  }
+
+  /// Parses the cache manifest and returns the list of previously-cached
+  /// phases.
+  ///
+  /// If the manifest indicates that the SDK version is out-of-date, this
+  /// deletes the existing cache. Otherwise, 
+  List<Set<TransformerId>> _parseManifest() {
+    if (!fileExists(_manifestPath)) return [];
+
+    var manifest = readTextFile(_manifestPath).split("\n");
+
+    // The first line of the manifest is the SDK version. We want to clear out
+    // the snapshots even if they're VM-compatible, since pub's transformer
+    // isolate scaffolding may have changed.
+    if (manifest.removeAt(0) != sdk.version.toString()) {
+      deleteEntry(_dir);
+      return [];
+    }
+
+    // The remaining lines of the manifest are comma-separated lists of
+    // transformer ids in each snapshotted phase. We can re-use phases with

[Bob Nystrom, 2014/09/10 17:58:09]

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.

[nweiz, 2014/09/10 23:30:12]

Done.

+    // the same ids, but not those that have changed.
+    //
+    // Normally we wouldn't cache something that's dependent on the
+    // entrypoint's pubspec, but in this case it's unlikely that the set or
+    // order of transformers will change frequently and we want to load them
+    // as quickly as possible.
+    return manifest.map((line) {
+      return line.split(",").map((id) => new TransformerId.parse(id, null))
+          .toSet();
+    }).toList();
+  }
+}