Patchwork [4,of,5,V2] manifest: use property instead of field for manifest revlog storage

login
register
mail settings
Submitter Durham Goode
Date Aug. 17, 2016, 9:07 p.m.
Message ID <d8456d11c582e4b073ed.1471468024@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/16343/
State Accepted
Headers show

Comments

Durham Goode - Aug. 17, 2016, 9:07 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1471465513 25200
#      Wed Aug 17 13:25:13 2016 -0700
# Node ID d8456d11c582e4b073ed9a7bf8098ee4393df5ca
# Parent  00f8d3832aad368660c69eff4be3e03a5568aebe
manifest: use property instead of field for manifest revlog storage

The file caches we're using to avoid reloading the manifest from disk everytime
has an annoying bug that causes the in memory structure to not be reloaded if
the mtime and the size haven't changed. This causes a breakage in the tests
because the manifestlog is not being reloaded after a commit+strip operation in
mq (the mtime is the same because it all happens in the same second, and the
resulting size is the same because we add 1 and remove 1). The only reason this
doesn't affect the manifest itself is because we touch it so often that we
had already reloaded it after the commit, but before the strip.

Once the entire manifest has migrated to manifestlog, we can get rid of these
properties, since then the manifestlog will be touched after the commit, but
before the strip, as well.
via Mercurial-devel - Aug. 31, 2016, 5:07 p.m.
On Wed, Aug 17, 2016 at 2:07 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1471465513 25200
> #      Wed Aug 17 13:25:13 2016 -0700
> # Node ID d8456d11c582e4b073ed9a7bf8098ee4393df5ca
> # Parent  00f8d3832aad368660c69eff4be3e03a5568aebe
> manifest: use property instead of field for manifest revlog storage
>
> The file caches we're using to avoid reloading the manifest from disk everytime
> has an annoying bug that causes the in memory structure to not be reloaded if
> the mtime and the size haven't changed. This causes a breakage in the tests
> because the manifestlog is not being reloaded after a commit+strip operation in
> mq (the mtime is the same because it all happens in the same second, and the
> resulting size is the same because we add 1 and remove 1). The only reason this
> doesn't affect the manifest itself is because we touch it so often that we
> had already reloaded it after the commit, but before the strip.
>
> Once the entire manifest has migrated to manifestlog, we can get rid of these
> properties, since then the manifestlog will be touched after the commit, but
> before the strip, as well.
>
> 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):
>      def manifest(self):
>          return manifest.manifest(self.svfs)
>
> -    @storecache('00manifest.i')
> +    @property
>      def manifestlog(self):
> -        return manifest.manifestlog(self.svfs, self.manifest)
> +        return manifest.manifestlog(self.svfs, self)
>
>      @repofilecache('dirstate')
>      def dirstate(self):
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -922,17 +922,23 @@ class manifestlog(object):
>      of the list of files in the given commit. Consumers of the output of this
>      class do not care about the implementation details of the actual manifests
>      they receive (i.e. tree or flat or lazily loaded, etc)."""
> -    def __init__(self, opener, oldmanifest):
> -        self._revlog = oldmanifest
> +    def __init__(self, opener, repo):
> +        self._repo = repo

I didn't notice until now, but I'm pretty sure this is a layering
violation: the revlogs are not supposed to know about the repo. Will
this have gone away by the end of your refactoring?

>
>          # We'll separate this into it's own cache once oldmanifest is no longer
>          # used
> -        self._mancache = oldmanifest._mancache
> +        self._mancache = repo.manifest._mancache
>
> +    @property
> +    def _revlog(self):
> +        return self._repo.manifest
> +
> +    @property
> +    def _oldmanifest(self):
>          # _revlog is the same as _oldmanifest right now, but we eventually want
>          # to delete _oldmanifest while still allowing manifestlog to access the
>          # revlog specific apis.
> -        self._oldmanifest = oldmanifest
> +        return self._repo.manifest
>
>      def __getitem__(self, node):
>          """Retrieves the manifest instance for the given node. Throws a KeyError
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - Aug. 31, 2016, 5:44 p.m.
On 8/31/16 10:07 AM, Martin von Zweigbergk wrote:
> On Wed, Aug 17, 2016 at 2:07 PM, Durham Goode <durham@fb.com> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1471465513 25200
>> #      Wed Aug 17 13:25:13 2016 -0700
>> # Node ID d8456d11c582e4b073ed9a7bf8098ee4393df5ca
>> # Parent  00f8d3832aad368660c69eff4be3e03a5568aebe
>> manifest: use property instead of field for manifest revlog storage
>>
>> The file caches we're using to avoid reloading the manifest from disk everytime
>> has an annoying bug that causes the in memory structure to not be reloaded if
>> the mtime and the size haven't changed. This causes a breakage in the tests
>> because the manifestlog is not being reloaded after a commit+strip operation in
>> mq (the mtime is the same because it all happens in the same second, and the
>> resulting size is the same because we add 1 and remove 1). The only reason this
>> doesn't affect the manifest itself is because we touch it so often that we
>> had already reloaded it after the commit, but before the strip.
>>
>> Once the entire manifest has migrated to manifestlog, we can get rid of these
>> properties, since then the manifestlog will be touched after the commit, but
>> before the strip, as well.
>>
>> 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):
>>       def manifest(self):
>>           return manifest.manifest(self.svfs)
>>
>> -    @storecache('00manifest.i')
>> +    @property
>>       def manifestlog(self):
>> -        return manifest.manifestlog(self.svfs, self.manifest)
>> +        return manifest.manifestlog(self.svfs, self)
>>
>>       @repofilecache('dirstate')
>>       def dirstate(self):
>> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
>> --- a/mercurial/manifest.py
>> +++ b/mercurial/manifest.py
>> @@ -922,17 +922,23 @@ class manifestlog(object):
>>       of the list of files in the given commit. Consumers of the output of this
>>       class do not care about the implementation details of the actual manifests
>>       they receive (i.e. tree or flat or lazily loaded, etc)."""
>> -    def __init__(self, opener, oldmanifest):
>> -        self._revlog = oldmanifest
>> +    def __init__(self, opener, repo):
>> +        self._repo = repo
> I didn't notice until now, but I'm pretty sure this is a layering
> violation: the revlogs are not supposed to know about the repo. Will
> this have gone away by the end of your refactoring?
Yes, this is just here to allow reusing the manifest from the repo so we 
avoid the cache invalidation bug.  One of the last patches in the series 
removes this.

Patch

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):
     def manifest(self):
         return manifest.manifest(self.svfs)
 
-    @storecache('00manifest.i')
+    @property
     def manifestlog(self):
-        return manifest.manifestlog(self.svfs, self.manifest)
+        return manifest.manifestlog(self.svfs, self)
 
     @repofilecache('dirstate')
     def dirstate(self):
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -922,17 +922,23 @@  class manifestlog(object):
     of the list of files in the given commit. Consumers of the output of this
     class do not care about the implementation details of the actual manifests
     they receive (i.e. tree or flat or lazily loaded, etc)."""
-    def __init__(self, opener, oldmanifest):
-        self._revlog = oldmanifest
+    def __init__(self, opener, repo):
+        self._repo = repo
 
         # We'll separate this into it's own cache once oldmanifest is no longer
         # used
-        self._mancache = oldmanifest._mancache
+        self._mancache = repo.manifest._mancache
 
+    @property
+    def _revlog(self):
+        return self._repo.manifest
+
+    @property
+    def _oldmanifest(self):
         # _revlog is the same as _oldmanifest right now, but we eventually want
         # to delete _oldmanifest while still allowing manifestlog to access the
         # revlog specific apis.
-        self._oldmanifest = oldmanifest
+        return self._repo.manifest
 
     def __getitem__(self, node):
         """Retrieves the manifest instance for the given node. Throws a KeyError