Patchwork D1577: localrepo: fix cache repo._filteredrepotypes by adding filtername in key

login
register
mail settings
Submitter phabricator
Date Dec. 2, 2017, 12:32 a.m.
Message ID <differential-rev-PHID-DREV-mqbquedfqsv7ldx36ck7-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25875/
State Superseded
Headers show

Comments

phabricator - Dec. 2, 2017, 12:32 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Before this patch,
  
  In [13]: repo.filtered('visible').__class__
  Out[13]: mercurial.localrepo.visiblefilteredrepo
  
  In [14]: repo.filtered('served').__class__
  Out[14]: mercurial.localrepo.visiblefilteredrepo
  
  `repo.unfiltered().__class__` can be same for two different repoviews, so that's
  not a good key for the cache. We need to include the filtername in the key so
  that we can have different keys for different repoview classes. After this
  patch,
  
  In [13]: repo.filtered('visible').__class__
  Out[13]: mercurial.localrepo.visiblefilteredrepo
  
  In [14]: repo.filtered('served').__class__
  Out[14]: mercurial.localrepo.servedfilteredrepo
  
  The above behaviour can also be noticed using `hg debugshell`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

AFFECTED FILES
  mercurial/localrepo.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 2, 2017, 3:24 a.m.
yuja added subscribers: indygreg, quark, yuja.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  @quark, @indygreg
  
  Is there any practical problem other than the wrong class name?
  
  IIUC, the composed type object shouldn't include a filter name, as it is
  an instance-level property.

INLINE COMMENTS

> localrepo.py:689
> +        unfibase = self.unfiltered().__class__
> +        key = str(self.unfiltered().__class__) + name
>          if key not in self._filteredrepotypes:

Perhaps a tuple of `(cls,  filter name)` is a better key if we have
to create a type object per filter.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel
phabricator - Dec. 2, 2017, 7:01 a.m.
quark added a comment.


  This was my idea to support the "directaccess" exception rules. Ideally, the exception state should be per `(unfiltered repo, filter name)`. So the `cls` here is a good candidate to attach that exception revs. (ex. `cls._visibleexceptionrevs = set()`).
  
  More formally, maybe we should create different real classes for different repoview filters. The current way seems obscure: it basically turns a "filter func" into a class here and has cache at scattered places (ex. the class cache, filteredrevs cache and changelog object cache) with obscure cache invalidate logic (repoview.changelog). Moving the filtered revs cache and filter function to those filter classes sounds cleaner to me. But I'm not sure if that should block pulkit's directaccess series or not.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel
phabricator - Dec. 2, 2017, 2:03 p.m.
yuja added a comment.


  > So the cls here is a good candidate to attach that exception revs.
  >  (ex. cls._visibleexceptionrevs = set()).
  
  Do you mean a proxy class will be defined per filter? e.g.
  
    class visiblerepoview(repoview, repocls):
        # impl specific to "visible"
    
    class servedrepoview(repoview, repocls):
        # impl specific to "served"
  
  This patch itself doesn't make much sense right now because the proxy
  class has no implementation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel
phabricator - Dec. 3, 2017, 1:36 a.m.
quark added a comment.


  Yes. I think the proxy class can have all states related to the filter. For example, `unfi.filteredrevcache[filtername]` could be `unfi._filteredrepotypes[typename + filtername]._revcache`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel
phabricator - Dec. 3, 2017, 9:10 a.m.
yuja added a comment.


  In https://phab.mercurial-scm.org/D1577#26858, @quark wrote:
  
  > Yes. I think the proxy class can have all states related to the filter. For example, `unfi.filteredrevcache[filtername]` could be `unfi._filteredrepotypes[typename + filtername]._revcache`.
  
  
  Sorry I can't see why we need a separate "type" object in this example.
  Isn't that effectively a cache of filtered instances?
  
  Anyway, I don't think this patch is the right fix as itself. @pulkit, can you
  send a series of patches which really depend on per-filter proxy class?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel
phabricator - Dec. 3, 2017, 8:10 p.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1577#26870, @yuja wrote:
  
  > Sorry I can't see why we need a separate "type" object in this example.
  >  Isn't that effectively a cache of filtered instances?
  
  
  Alternatively, "visible exception revs" could be stored as `unfi._exceptions[filtername]`. That does not require creating multiple types.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel
phabricator - Dec. 4, 2017, 2:24 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D1577#26833, @yuja wrote:
  
  > Is there any practical problem other than the wrong class name?
  
  
  There was some problem at the time when I send the patch but that got solved. So no problem apart from that.
  
  > IIUC, the composed type object shouldn't include a filter name, as it is
  >  an instance-level property.
  
  Yes, this part confused me a lot while debugging. When I see what repo object I has, I get `mercurial.localrepo.visiblefilteredrepo` and I assume it's filtered by `visible` filter but later after going through filteredrevs, got to know that the name of type object can be wrong.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1577

To: pulkit, #hg-reviewers, yuja
Cc: yuja, quark, indygreg, mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -503,7 +503,7 @@ 
         self._postdsstatus = []
 
         # Cache of types representing filtered repos.
-        self._filteredrepotypes = weakref.WeakKeyDictionary()
+        self._filteredrepotypes = weakref.WeakValueDictionary()
 
         # generic mapping between names and nodes
         self.names = namespaces.namespaces()
@@ -685,12 +685,13 @@ 
         # created types so this method doesn't leak on every
         # invocation.
 
-        key = self.unfiltered().__class__
+        unfibase = self.unfiltered().__class__
+        key = str(self.unfiltered().__class__) + name
         if key not in self._filteredrepotypes:
             # Build a new type with the repoview mixin and the base
             # class of this repo. Give it a name containing the
             # filter name to aid debugging.
-            bases = (repoview.repoview, key)
+            bases = (repoview.repoview, unfibase)
             cls = type(r'%sfilteredrepo' % name, bases, {})
             self._filteredrepotypes[key] = cls