API for the new AnalysisDriver.

pkg/analyzer/lib/src/dart/analysis/byte_store.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.
+
+import 'dart:collection';
+
+/**
+ * Store of bytes associated with string keys.
+ *
+ * Note that associations are not guaranteed to be persistent. The value
+ * associated with a key can change or become `null` at any point of time.

[Paul Berry, 2016/10/23 10:39:19]

s/of/in/

[scheglov, 2016/10/23 20:54:34]

Done.

+ */
+abstract class ByteStore {

[Paul Berry, 2016/10/23 10:39:19]

Since this is a synchronous interface, it means that it would be impossible to
write implementations of [ByteStore] in the future that:
(a) share data across isolates.
(b) store data in a remote location (such as a cache located on a machine that
is shared by multiple users).

Would it be worth switching this to an asynchronous interface?  Since
AnalysisDriver is already asynchronous it sounds like we would not lose much. 
And the future benefits might be really big.

[scheglov, 2016/10/23 20:54:33]

I thought about this.

Maybe even global, LAN or company wide distributed analysis cache.

And I wrote it this way initially.
And tested performance difference between reading files sync and async.
If we use async File.readAsBytes(), it takes 3 times longer to read 500 files
than when we use File.readAsBytesSync(). OTOH, we can cheat and provide async
interface using sync implementation. Then the difference is small, IIRC about
10%.

I will switch it to async once I will have some basic implementation.
This will cause some ripples, but probably not too bad.

[Brian Wilkerson, 2016/10/25 17:24:12]

Those performance measures are consistent with Bob's earlier findings.

+  /**
+   * Return the bytes associated with the given [key].
+   * Return `null` if the association does not exist.
+   */
+  List<int> get(String key);

[Paul Berry, 2016/10/23 10:39:19]

Since FileByteStore uses the key as a filename, we should document that the key
should be suitable for use as a filename (i.e. shouldn't be too long, shouldn't
contain characters like "/" or "\", shouldn't be "." or "..", etc.)

[scheglov, 2016/10/23 20:54:33]

Done.

In theory as long as keys are short enough, it does not actually matters what
they consist of - we can always transform keys into a form that is appropriate
for the medium - file system, key/value store, etc.

But we might never need this flexibility so yes, let's set limits.

+
+  /**
+   * Associate the given [bytes] with the [key].
+   */
+  void put(String key, List<int> bytes);
+}
+
+/**
+ * [ByteStore] with LRU cache.

[Paul Berry, 2016/10/23 10:39:19]

When I first read this comment it wasn't clear to me whether this was (a) an
implementation of [ByteStore] that stores data purely in memory, and discards it
when it is not recently used, or (b) a wrapper around another [ByteStore] which
adds in-memory LRU caching behavior to it.  I now see that it's (b).

Maybe change the comment to something like `A wrapper around [ByteStore] which
adds an in-memory LRU cache to it.`

[scheglov, 2016/10/23 20:54:34]

Done.

+ */
+class MemoryCachingByteStore implements ByteStore {
+  final ByteStore store;
+  final int maxEntries;

[Paul Berry, 2016/10/23 10:39:19]

As a future improvement, consider measuring the limit in total stored bytes
rather than number of entries.  That way we will be able to make much stronger
guarantees about memory usage.

[scheglov, 2016/10/23 20:54:34]

Agreed.
We could look at Google Guava for inspiration :-)
https://github.com/google/guava/wiki/CachesExplained

+
+  final _map = <String, List<int>>{};
+  final _keys = new LinkedHashSet<String>();
+
+  MemoryCachingByteStore(this.store, this.maxEntries);
+
+  @override
+  List<int> get(String key) {
+    _keys.remove(key);
+    _keys.add(key);
+    _evict();
+    return _map.putIfAbsent(key, () => store.get(key));
+  }
+
+  @override
+  void put(String key, List<int> bytes) {

[Brian Wilkerson, 2016/10/25 17:24:12]

Is there a need to be able to remove items from the store? As it is, the _keys
and _map collections can only grow in size, which sounds like a memory leak in
server.

[scheglov, 2016/10/25 18:38:45]

AFAIK _evict() ensures that there is a limit on the number of elements in _keys
and _map.

+    store.put(key, bytes);
+    _map[key] = bytes;
+    _keys.add(key);
+    _evict();
+  }
+
+  void _evict() {
+    if (_keys.length > maxEntries) {
+      String key = _keys.first;
+      _keys.remove(key);
+      _map.remove(key);
+    }
+  }
+}