cache pub list results

pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart

+// Copyright (c) 2015, 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 source.caching_pub_package_map_provider;
+
+import 'dart:convert';
+import 'dart:io' as io;
+
+import 'package:analyzer/file_system/file_system.dart';
+import 'package:analyzer/source/package_map_provider.dart';
+import 'package:analyzer/source/pub_package_map_provider.dart';
+import 'package:analyzer/src/generated/engine.dart';
+import 'package:analyzer/src/generated/sdk_io.dart';
+import 'package:analyzer/src/generated/source.dart';
+
+/**
+ * The function used to write the cache file.
+ * Returns the modification timestamp for the newly written file.
+ */
+typedef int WriteFile(File file, String content);
+
+/**
+ * [PubPackageMapProvider] extension which caches pub list results.
+ * These results are cached in memory and in a single place on disk that is
+ * shared cross session and between different simultaneous sessions.
+ */
+class CachingPubPackageMapProvider extends PubPackageMapProvider {
+  static const cacheKey = 'pub_list_cache';
+  static const cacheVersion = 1;
+  static const cacheVersionKey = 'pub_list_cache_version';
+  static const pubListResultKey = 'pub_list_result';
+  static const modificationStampsKey = 'modification_stamps';
+
+  /**
+   * A cache of folder path to pub list information as shown below
+   * or `null` if the cache has not yet been initialized.
+   *
+   *     {
+   *       "path/to/folder": {
+   *         "pub_list_result": {
+   *           "packages": {
+   *             "foo": "path/to/foo",
+   *             "bar": ["path/to/bar1", "path/to/bar2"],
+   *             "myapp": "path/to/myapp",  // self link is included
+   *           },
+   *           "input_files": [
+   *             "path/to/myapp/pubspec.lock"
+   *           ]
+   *         },
+   *         "modification_stamps": {
+   *           "path/to/myapp/pubspec.lock": 1424305309
+   *         }
+   *       }
+   *       "path/to/another/folder": {
+   *         ...
+   *       }
+   *       ...
+   *     }
+   */
+  Map<String, Map> _cache;
+
+  /**
+   * The modification time of the cache file when it was last read

[Paul Berry, 2015/02/20 22:00:25]

s/read/accessed/g

We update the modification time when writing the cache too.

[danrubel, 2015/02/21 03:09:30]

Done.

+   * or `null` if it has not yet been read.
+   */
+  int _cacheModificationTime;
+
+  /**
+   * The function used to write the cache file.
+   */
+  WriteFile _writeFile;
+
+  /**
+   * Construct a new instance.
+   * [RunPubList] and [WriteFile] implementations may be injected for testing
+   */
+  CachingPubPackageMapProvider(ResourceProvider resourceProvider,
+      DirectoryBasedDartSdk sdk, [RunPubList runPubList, this._writeFile])
+      : super(resourceProvider, sdk, runPubList) {
+    if (_writeFile == null) {
+      _writeFile = _writeFileDefault;
+    }
+  }
+
+  File get _cacheFile =>
+      resourceProvider.getStateLocation('.pub-list').getChild('cache');
+
+  @override
+  PackageMapInfo computePackageMap(Folder folder) {
+    if (!folder.exists) {
+      return computePackageMapError(folder);
+    }
+    // Ensure cache is up to date
+    _readCache();
+    // Check for cached entry
+    Map entry = _cache[folder.path];
+    if (entry != null) {
+      Map<String, int> modificationStamps = entry[modificationStampsKey];
+      if (modificationStamps != null) {
+        //
+        // Check to see if any dependencies have changed
+        // before returning cached result
+        //
+        bool dependencyChanged = false;
+        modificationStamps.forEach((String path, int modificationStamp) {

[Paul Berry, 2015/02/20 22:00:26]

I'd recommend changing this to an ordinary "for" loop so that you can break out
of it early if a dependency is found that has changed.

[danrubel, 2015/02/21 03:09:30]

That would require a hash lookup on each iteration.
Which is more efficient?

[Paul Berry, 2015/02/23 18:54:58]

Since calling res.exists and res.createSource().modificationStamp both require
hitting the filesystem, I believe that the extra hash lookup will be tiny by
comparison.

[danrubel, 2015/02/24 02:29:45]

Good point. Done.

+          Resource res = resourceProvider.getResource(path);
+          if (res is File) {
+            if (!res.exists ||
+                res.createSource().modificationStamp != modificationStamp) {
+              dependencyChanged = true;
+            }
+          } else {
+            dependencyChanged = true;
+          }
+        });
+        if (!dependencyChanged) {
+          return parsePackageMap(entry[pubListResultKey], folder);
+        }
+      }
+    }
+    // Create a new entry if one does not already exist

[Paul Berry, 2015/02/20 22:00:25]

It's risky to do this here, since it leaves _cache[folder.path] in an invalid
state which will persist if an exception gets thrown.  I'd suggest deleting this
if-block and doing the update to the cache in a single atomic operation (see
suggestion below on line 144).

[danrubel, 2015/02/21 03:09:30]

Done.

+    if (entry == null) {
+      entry = new Map<String, Map>();
+      _cache[folder.path] = entry;
+    }
+    // computePackageMap calls parsePackageMap which caches the result
+    PackageMapInfo info = super.computePackageMap(folder);
+    _writeCache();
+    return info;
+  }
+
+  @override
+  PackageMapInfo parsePackageMap(Map obj, Folder folder) {
+    PackageMapInfo info = super.parsePackageMap(obj, folder);
+    Map<String, int> modificationStamps = new Map<String, int>();

[Paul Berry, 2015/02/20 22:00:26]

Nit:  I believe the standard dart idiom for creating an empty map is

<String, int>{}

instead of 

new Map<String, int>()

(this is a really minor complaint though--they should compile down to the exact
same CPU instructions)

[Brian Wilkerson, 2015/02/20 22:05:36]

True, but what we really want, for efficiency's sake, is

  new HashMap<String, int>()

+    for (String path in info.dependencies) {
+      Resource res = resourceProvider.getResource(path);
+      if (res is File && res.exists) {
+        modificationStamps[path] = res.createSource().modificationStamp;
+      }
+    }
+    // Assumes entry has been initialized by computePackageMap
+    Map entry = _cache[folder.path];

[Paul Berry, 2015/02/20 22:00:25]

These three lines can be replaced with a single atomic update:

_cache[folder.path] = <String, Map>{ pubListResultKey: obj,
modificationStampsKey: modificationStamps };

[danrubel, 2015/02/21 03:09:30]

Great suggestion. Done.

+    entry[pubListResultKey] = obj;
+    entry[modificationStampsKey] = modificationStamps;
+    return info;
+  }
+
+  /**
+   * Read the cache from disk if it has not been read before.
+   */
+  void _readCache() {
+    // TODO(danrubel) This implementation assumes that
+    // two separate processes are not accessing the cache file at the same time
+    Source source = _cacheFile.createSource();
+    if (source.exists() &&
+        (_cache == null || _cacheModificationTime != source.modificationStamp)) {
+      TimestampedData<String> data = source.contents;
+      Map map = JSON.decode(data.data);
+      if (map[cacheVersionKey] == cacheVersion) {
+        _cache = map[cacheKey];
+        _cacheModificationTime = data.modificationTime;
+      }
+    }
+    if (_cache == null) {
+      _cache = new Map<String, Map>();
+    }
+  }
+
+  /**
+   * Write the cache to disk.
+   */
+  void _writeCache() {
+    _cacheModificationTime = _writeFile(_cacheFile, JSON.encode({
+      cacheVersionKey: cacheVersion,
+      cacheKey: _cache
+    }));
+  }
+
+  /**
+   * Update the given file with the specified content.
+   */
+  int _writeFileDefault(File cacheFile, String content) {
+    // TODO(danrubel) This implementation assumes that
+    // two separate processes are not accessing the cache file at the same time
+    io.File file = new io.File(cacheFile.path);
+    if (!file.parent.existsSync()) {
+      file.parent.createSync(recursive: true);
+    }
+    file.writeAsStringSync(content, flush: true);
+    return file.lastModifiedSync().millisecondsSinceEpoch;
+  }
+}