fastmanifest: fix logic bug in the cache

Summary:
This patch adds a test that uncovers logic bugs in the cache. It also
adds fixes for these bugs.

Test Plan:
Add a new test and ran existing tests, observed no change for
exisiting test.

Reviewers: durham, ttung

Subscribers: mjpieters

Differential Revision: https://phabricator.intern.facebook.com/D3450430

fastmanifest/implementation.py

@@ -464,6 +464,8 @@ class ondiskcache(object):
         return iter(self.items())
 
     def setwithlimit(self, hexnode, manifest, limit=-1):
+        if hexnode in self:
+            return
         path = self._pathfromnode(hexnode)
         fm = cfastmanifest.fastmanifest(manifest.text())
         tmpfpath = util.mktempcopy(path, True)
@@ -566,7 +568,7 @@ class fastmanifestcache(object):
         return hexnode in self.inmemorycache or hexnode in self.ondiskcache
 
     def __setitem__(self, hexnode, manifest):
-        if hexnode in self:
+        if hexnode in self.ondiskcache and hexnode in self.inmemorycache:
             self.debug("[FM] skipped %s, already cached\n" % hexnode)
             return
 
@@ -582,6 +584,7 @@ class fastmanifestcache(object):
         else:
             self.debug("[FM] caching revision %s\n" % hexnode)
             self.ondiskcache[hexnode] = manifest
+        self.put_inmemory(hexnode, manifest)
 
     def put_inmemory(self, hexnode, fmdict):
         if hexnode not in self.inmemorycache:

tests/test-fastmanifest.py

@@ -10,6 +10,49 @@ from mercurial import scmutil
 from mercurial import ui
 from mercurial import util
 
+class mockmanifest(object):
+    def __init__(self, text):
+        self.text = text
+
+    def copy(self):
+        return mockmanifest(self.text)
+
+class mockondiskcache(object):
+    def __init__(self):
+        self.data = {}
+
+    def _pathfromnode(self, hexnode):
+        return hexnode
+
+    def touch(self, hexnode, delay=0):
+        pass
+
+    def __contains__(self, hexnode):
+        return hexnode in self.data
+
+    def items(self):
+        return self.data.keys()
+
+    def __iter__(self):
+        return iter(self.items())
+
+    def __setitem__(self, hexnode, manifest):
+        self.data[hexnode] = manifest
+
+    def __delitem__(self, hexnode):
+        if hexnode in self.data:
+            del self.data[hexnode]
+
+    def __getitem__(self, hexnode):
+        return self.data.get(hexnode, None)
+
+    def entrysize(self, hexnode):
+        return len(self.data[hexnode]) if hexnode in self.data else None
+
+    def totalsize(self, silent=True):
+        return (sum(self.entrysize(hexnode) for hexnode in self),
+                len(self.items()))
+
 class HybridManifest(unittest.TestCase):
 
     def test_wrap(self):
@@ -63,6 +106,43 @@ class HybridManifest(unittest.TestCase):
         self.assertRaises(OSError,
                           lambda: vfs.lstat("lock"))
 
+    def test_cachehierarchy(self):
+        """We mock the ondisk cache and test that the two layers of cache
+        work properly"""
+        vfs = scmutil.vfs(os.getcwd())
+        cache = fastmanifest.implementation.fastmanifestcache(vfs,
+                                                              ui.ui(), None)
+        ondiskcache = mockondiskcache()
+        cache.ondiskcache = ondiskcache
+        # Test 1) Put one manifest in the cache, check that retrieving it does
+        # not hit the disk
+        cache["abc"] = mockmanifest("abcnode")
+        # remove the ondiskcache to make sure we don't hit it
+        cache.ondiskcache = None
+        assert cache["abc"].text == "abcnode"
+        assert ondiskcache.data["abc"].text == "abcnode"
+        cache.ondiskcache = ondiskcache
+
+        # Test 2) Put an entry in the cache that is already in memory but not
+        # on disk, should write it on disk
+        ondiskcache.data.clear()
+        cache["abc"] = mockmanifest("abcnode")
+        assert ondiskcache.data["abc"].text == "abcnode"
+
+        # Test 3) Put an entry in the cache that is already on disk, not in
+        # memory, it should be added to the inmemorycache
+        cache.inmemorycache.clear()
+        cache["abc"] = mockmanifest("abcnode")
+        assert cache.inmemorycache["abc"].text == "abcnode"
+
+        # Test 4) We have at most 10 entries in the in memorycache by
+        # default
+        for a in range(20):
+            cache[chr(a + ord('a'))] = mockmanifest(chr(a + ord('a')) + "node")
+
+        assert len(cache.ondiskcache.items()) == 21
+        assert len(cache.inmemorycache) == 10
+
     def test_looselock_stealing(self):
         """Attempt to secure three locks.  The second lock should succeed
         through a steal.  The third lock should fail because the second lock