Page MenuHomePhabricator

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

Authored by pulkit on Dec 1 2017, 7:32 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Dec 1 2017, 7:32 PM
yuja requested changes to this revision.

@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.

mercurial/localrepo.py
689

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

This revision now requires changes to proceed.Dec 1 2017, 10:24 PM
quark added a comment.EditedDec 2 2017, 2:01 AM

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 (esp. 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.

yuja added a comment.Dec 2 2017, 9:03 AM

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.

quark added a comment.Dec 2 2017, 8:36 PM

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.

yuja added a comment.Dec 3 2017, 4:10 AM
In 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?

quark added a comment.Dec 3 2017, 3:10 PM
In 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.

pulkit added a comment.Dec 4 2017, 9:24 AM
In 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.

pulkit abandoned this revision.Jan 10 2018, 3:53 AM