This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1577

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

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

Download Raw Diff

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptDec 1 2017, 7:32 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

@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

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.

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.

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.

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?

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.

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

Revision Contents
Changeset List

			Path	Packages
M			mercurial/localrepo.py (7 lines)

Diff 4067

mercurial/localrepo.py

	# - working directory parent change,			# - working directory parent change,
	# - bookmark changes			# - bookmark changes
	self.filteredrevcache = {}			self.filteredrevcache = {}

	# post-dirstate-status hooks			# post-dirstate-status hooks
	self._postdsstatus = []			self._postdsstatus = []

	# Cache of types representing filtered repos.			# Cache of types representing filtered repos.
	self._filteredrepotypes = weakref.WeakKeyDictionary()			self._filteredrepotypes = weakref.WeakValueDictionary()

	# generic mapping between names and nodes			# generic mapping between names and nodes
	self.names = namespaces.namespaces()			self.names = namespaces.namespaces()

	# Key to signature value.			# Key to signature value.
	self._sparsesignaturecache = {}			self._sparsesignaturecache = {}
	# Signature to cached matcher instance.			# Signature to cached matcher instance.
	self._sparsematchercache = {}			self._sparsematchercache = {}

	def filtered(self, name):			def filtered(self, name):
	"""Return a filtered version of a repository"""			"""Return a filtered version of a repository"""
	# Python <3.4 easily leaks types via __mro__. See			# Python <3.4 easily leaks types via __mro__. See
	# https://bugs.python.org/issue17950. We cache dynamically			# https://bugs.python.org/issue17950. We cache dynamically
	# created types so this method doesn't leak on every			# created types so this method doesn't leak on every
	# invocation.			# invocation.

	key = self.unfiltered().__class__			unfibase = self.unfiltered().__class__
				key = str(self.unfiltered().__class__) + name
				yujaUnsubmitted Not Done Perhaps a tuple of `(cls, filter name)` is a better key if we have to create a type object per filter. yuja: Perhaps a tuple of `(cls, filter name)` is a better key if we have to create a type object per…
	if key not in self._filteredrepotypes:			if key not in self._filteredrepotypes:
	# Build a new type with the repoview mixin and the base			# Build a new type with the repoview mixin and the base
	# class of this repo. Give it a name containing the			# class of this repo. Give it a name containing the
	# filter name to aid debugging.			# filter name to aid debugging.
	bases = (repoview.repoview, key)			bases = (repoview.repoview, unfibase)
	cls = type(r'%sfilteredrepo' % name, bases, {})			cls = type(r'%sfilteredrepo' % name, bases, {})
	self._filteredrepotypes[key] = cls			self._filteredrepotypes[key] = cls

	return self._filteredrepotypes[key](self, name)			return self._filteredrepotypes[key](self, name)

	@repofilecache('bookmarks', 'bookmarks.current')			@repofilecache('bookmarks', 'bookmarks.current')
	def _bookmarks(self):			def _bookmarks(self):
	return bookmarks.bmstore(self)			return bookmarks.bmstore(self)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	4067			Dec 1 2017, 7:32 PM	★	★