Patchwork [2,of,4,STABLE] manifest: make manifestlog a storecache

login
register
mail settings
Submitter Durham Goode
Date Oct. 19, 2016, 12:50 a.m.
Message ID <ed607426a3ff4deda8c7.1476838215@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17176/
State Accepted
Headers show

Comments

Durham Goode - Oct. 19, 2016, 12:50 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1476837219 25200
#      Tue Oct 18 17:33:39 2016 -0700
# Branch stable
# Node ID ed607426a3ff4deda8c7f2de8b86d5b6ca976d67
# Parent  f4e70498d617737c47371a86c2177146c7b789fe
manifest: make manifestlog a storecache

The old @property on manifestlog was broken. It meant that we would always
recreate the manifestlog instance, which meant the cache was never hit. Since
we'll eventually remove repo.manifest and make manifestlog the only property,
let's go ahead and make manifestlog the @storecache property, have manifestlog
own the manifest instance, and have repo.manifest refer to it via manifestlog.

This means all accesses go through repo.manifestlog, which is now invalidated
correctly.
Yuya Nishihara - Oct. 22, 2016, 9:09 a.m.
On Tue, 18 Oct 2016 17:50:15 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1476837219 25200
> #      Tue Oct 18 17:33:39 2016 -0700
> # Branch stable
> # Node ID ed607426a3ff4deda8c7f2de8b86d5b6ca976d67
> # Parent  f4e70498d617737c47371a86c2177146c7b789fe
> manifest: make manifestlog a storecache

> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -504,9 +504,9 @@ class localrepository(object):
>                  c.readpending('00changelog.i.a')
>          return c
>  
> -    @storecache('00manifest.i')
> +    @property
>      def manifest(self):
> -        return self._constructmanifest()
> +        return self.manifestlog._oldmanifest
>  
>      def _constructmanifest(self):
>          # This is a temporary function while we migrate from manifest to
> @@ -514,7 +514,7 @@ class localrepository(object):
>          # manifest creation.
>          return manifest.manifest(self.svfs)
>  
> -    @property
> +    @storecache('00manifest.i')
>      def manifestlog(self):
>          return manifest.manifestlog(self.svfs, self)

Can we remove manifestlog._repo to cut reference cycle?
Durham Goode - Oct. 25, 2016, 7:45 p.m.
On 10/22/16 2:09 AM, Yuya Nishihara wrote:
> On Tue, 18 Oct 2016 17:50:15 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1476837219 25200
>> #      Tue Oct 18 17:33:39 2016 -0700
>> # Branch stable
>> # Node ID ed607426a3ff4deda8c7f2de8b86d5b6ca976d67
>> # Parent  f4e70498d617737c47371a86c2177146c7b789fe
>> manifest: make manifestlog a storecache
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -504,9 +504,9 @@ class localrepository(object):
>>                   c.readpending('00changelog.i.a')
>>           return c
>>   
>> -    @storecache('00manifest.i')
>> +    @property
>>       def manifest(self):
>> -        return self._constructmanifest()
>> +        return self.manifestlog._oldmanifest
>>   
>>       def _constructmanifest(self):
>>           # This is a temporary function while we migrate from manifest to
>> @@ -514,7 +514,7 @@ class localrepository(object):
>>           # manifest creation.
>>           return manifest.manifest(self.svfs)
>>   
>> -    @property
>> +    @storecache('00manifest.i')
>>       def manifestlog(self):
>>           return manifest.manifestlog(self.svfs, self)
> Can we remove manifestlog._repo to cut reference cycle?
Right now everything that attempts to access the manifest must do it 
through the repo object.  So in order for ctx objects to read from the 
manifest, it has to have the repo object (which is why changectx has the 
repo object now).  Once we get rid of repo.manifest entirely, we can 
probably move the 00manifest.i caching down a layer into the 
manifestlog, and we could break the need to store the repo in the 
manifestlog.  I'll do that as part of the remainder of my manifest 
refactor series.

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -407,7 +407,7 @@  def perftags(ui, repo, **opts):
     repocleartagscache = repocleartagscachefunc(repo)
     def t():
         repo.changelog = mercurial.changelog.changelog(svfs)
-        repo.manifest = mercurial.manifest.manifest(svfs)
+        repo.manifestlog = mercurial.manifest.manifestlog(svfs, repo)
         repocleartagscache()
         return len(repo.tags())
     timer(t)
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -504,9 +504,9 @@  class localrepository(object):
                 c.readpending('00changelog.i.a')
         return c
 
-    @storecache('00manifest.i')
+    @property
     def manifest(self):
-        return self._constructmanifest()
+        return self.manifestlog._oldmanifest
 
     def _constructmanifest(self):
         # This is a temporary function while we migrate from manifest to
@@ -514,7 +514,7 @@  class localrepository(object):
         # manifest creation.
         return manifest.manifest(self.svfs)
 
-    @property
+    @storecache('00manifest.i')
     def manifestlog(self):
         return manifest.manifestlog(self.svfs, self)
 
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -1254,13 +1254,12 @@  class manifestlog(object):
             usetreemanifest = opts.get('treemanifest', usetreemanifest)
         self._treeinmem = usetreemanifest
 
+        self._oldmanifest = repo._constructmanifest()
+        self._revlog = self._oldmanifest
+
         # We'll separate this into it's own cache once oldmanifest is no longer
         # used
-        self._mancache = repo.manifest._mancache
-
-    @property
-    def _revlog(self):
-        return self._repo.manifest
+        self._mancache = self._oldmanifest._mancache
 
     def __getitem__(self, node):
         """Retrieves the manifest instance for the given node. Throws a KeyError
diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py
--- a/mercurial/statichttprepo.py
+++ b/mercurial/statichttprepo.py
@@ -155,7 +155,7 @@  class statichttprepository(localrepo.loc
         self._filecache = {}
         self.requirements = requirements
 
-        self.manifest = manifest.manifest(self.svfs)
+        self.manifestlog = manifest.manifestlog(self.svfs, self)
         self.changelog = changelog.changelog(self.svfs)
         self._tags = None
         self.nodetagscache = None