This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5638

branchmap: encapsulate cache updating in the map itself
ClosedPublic

Authored by mjpieters on Jan 21 2019, 1:08 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG328ca3b9e545: branchmap: encapsulate cache updating in the map itself
Summary

Rather than have a repository update the cache, move handling of cache updates
into the branchmap module, in the form of a custom mapping class.

This makes later performance improvements easier to handle too.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mjpieters created this revision.Jan 21 2019, 1:08 PM
martinvonz added a subscriber: martinvonz.Jan 25 2019, 3:52 PM
martinvonz added inline comments.
contrib/perf.py
2299

Same here: I think this needs to be made compatible with both versions (before and after this patch)

mercurial/localrepo.py
2077

Hmm, it's much less clear now that this updates the cache. At the very least, it deserves a comment. Is the updatecache() call from __getitem__() necessary for your later patches? Sorry, I didn't quite follow.

mercurial/streamclone.py
16

test-check-pyflakes.t says that this is now unused

mjpieters marked an inline comment as done.Jan 26 2019, 8:45 AM
mjpieters updated this revision to Diff 13502.
mjpieters added inline comments.Jan 26 2019, 8:46 AM
contrib/perf.py
2299

I'll wait for confirmation; see the other patch.

mercurial/localrepo.py
2077

Yes, the updating will change in a later, as yet to be submitted patch.

But why should it be clear here that there is a cache that is kept up-to-date on access to the branchmap? That's a responsibility of the cache, not of whatever accesses a branchmap. The cache is an implementation detail of branchmap, and should really not bleed out into code that merely consumes the map.

martinvonz added inline comments.Jan 26 2019, 9:45 AM
mercurial/localrepo.py
2077

Fair enough, it doesn't have to be clear that it updates the cache, but it has to be clear that it does *something*. It currently looks like a statement that can be removed, but I assume it cannot.

mjpieters updated this revision to Diff 13543.Jan 28 2019, 1:05 PM
mjpieters marked 2 inline comments as done.Jan 28 2019, 1:05 PM
pulkit added a subscriber: pulkit.Feb 7 2019, 3:29 PM

This looks good to me. @martinvonz do you have any concerns?

martinvonz added a comment.Feb 7 2019, 6:36 PM
In D5638#86131, @pulkit wrote:

This looks good to me. @martinvonz do you have any concerns?

Yes, looks good. Sorry, I thought this had already been queued.

pulkit accepted this revision.Feb 8 2019, 7:04 AM
Closed by commit rHG328ca3b9e545: branchmap: encapsulate cache updating in the map itself (authored by mjpieters). · Explain WhyFeb 8 2019, 7:44 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 13333

contrib/perf.py

else: else:
view = repo.filtered(filtername) view = repo.filtered(filtername)
def d(): def d():
if clear_revbranch: if clear_revbranch:
repo.revbranchcache()._clear() repo.revbranchcache()._clear()
if full: if full:
view._branchcaches.clear() view._branchcaches.clear()
else: else:
view._branchcaches.pop(filtername, None) view._branchcaches._per_filter.pop(filtername, None)
martinvonzUnsubmitted
Done

Same here: I think this needs to be made compatible with both versions (before and after this patch)

martinvonz: Same here: I think this needs to be made compatible with both versions (before and after this…
mjpietersAuthorUnsubmitted
Done

I'll wait for confirmation; see the other patch.

mjpieters: I'll wait for confirmation; see the other patch.
view.branchmap() view.branchmap()
return d return d
# add filter in smaller subset to bigger subset # add filter in smaller subset to bigger subset
possiblefilters = set(repoview.filtertable) possiblefilters = set(repoview.filtertable)
if filternames: if filternames:
possiblefilters &= set(filternames) possiblefilters &= set(filternames)
subsettable = getbranchmapsubsettable() subsettable = getbranchmapsubsettable()
allfilters = [] allfilters = []

mercurial/branchmap.py

# This create and ordering used for branchmap purpose. # This create and ordering used for branchmap purpose.
# the ordering may be partial # the ordering may be partial
subsettable = {None: 'visible', subsettable = {None: 'visible',
'visible-hidden': 'visible', 'visible-hidden': 'visible',
'visible': 'served', 'visible': 'served',
'served': 'immutable', 'served': 'immutable',
'immutable': 'base'} 'immutable': 'base'}
def updatecache(repo):
class BranchMapCache(object):
"""Cache mapping"""
def __init__(self):
self._per_filter = {}
def __getitem__(self, repo):
self.updatecache(repo)
return self._per_filter[repo.filtername]
def updatecache(self, repo):
"""Update the cache for the given filtered view on a repository""" """Update the cache for the given filtered view on a repository"""
# This can trigger updates for the caches for subsets of the filtered # This can trigger updates for the caches for subsets of the filtered
# view, e.g. when there is no cache for this filtered view or the cache # view, e.g. when there is no cache for this filtered view or the cache
# is stale. # is stale.
cl = repo.changelog cl = repo.changelog
filtername = repo.filtername filtername = repo.filtername
bcache = repo._branchcaches.get(filtername) bcache = self._per_filter.get(filtername)
if bcache is None or not bcache.validfor(repo): if bcache is None or not bcache.validfor(repo):
# cache object missing or cache object stale? Read from disk # cache object missing or cache object stale? Read from disk
bcache = branchcache.fromfile(repo) bcache = branchcache.fromfile(repo)
revs = [] revs = []
if bcache is None: if bcache is None:
# no (fresh) cache available anymore, perhaps we can re-use # no (fresh) cache available anymore, perhaps we can re-use
# the cache for a subset, then extend that to add info on missing # the cache for a subset, then extend that to add info on missing
# revisions. # revisions.
subsetname = subsettable.get(filtername) subsetname = subsettable.get(filtername)
if subsetname is not None: if subsetname is not None:
subset = repo.filtered(subsetname) subset = repo.filtered(subsetname)
bcache = subset.branchmap().copy() bcache = self[subset].copy()
extrarevs = subset.changelog.filteredrevs - cl.filteredrevs extrarevs = subset.changelog.filteredrevs - cl.filteredrevs
revs.extend(r for r in extrarevs if r <= bcache.tiprev) revs.extend(r for r in extrarevs if r <= bcache.tiprev)
else: else:
# nothing to fall back on, start empty. # nothing to fall back on, start empty.
bcache = branchcache() bcache = branchcache()
revs.extend(cl.revs(start=bcache.tiprev + 1)) revs.extend(cl.revs(start=bcache.tiprev + 1))
if revs: if revs:
bcache.update(repo, revs) bcache.update(repo, revs)
assert bcache.validfor(repo), filtername assert bcache.validfor(repo), filtername
repo._branchcaches[repo.filtername] = bcache self._per_filter[repo.filtername] = bcache
def replacecache(repo, bm): def replace(self, repo, remotebranchmap):
"""Replace the branchmap cache for a repo with a branch mapping. """Replace the branchmap cache for a repo with a branch mapping.
This is likely only called during clone with a branch map from a remote. This is likely only called during clone with a branch map from a
remote.
""" """
cl = repo.changelog cl = repo.changelog
clrev = cl.rev clrev = cl.rev
clbranchinfo = cl.branchinfo clbranchinfo = cl.branchinfo
rbheads = [] rbheads = []
closed = [] closed = []
for bheads in bm.itervalues(): for bheads in remotebranchmap.itervalues():
rbheads.extend(bheads) rbheads += bheads
for h in bheads: for h in bheads:
r = clrev(h) r = clrev(h)
b, c = clbranchinfo(r) b, c = clbranchinfo(r)
if c: if c:
closed.append(h) closed.append(h)
if rbheads: if rbheads:
rtiprev = max((int(clrev(node)) rtiprev = max((int(clrev(node)) for node in rbheads))
for node in rbheads)) cache = branchcache(
cache = branchcache(bm, remotebranchmap, repo[rtiprev].node(), rtiprev,
repo[rtiprev].node(),
rtiprev,
closednodes=closed) closednodes=closed)
# Try to stick it as low as possible # Try to stick it as low as possible
# filter above served are unlikely to be fetch from a clone # filter above served are unlikely to be fetch from a clone
for candidate in ('base', 'immutable', 'served'): for candidate in ('base', 'immutable', 'served'):
rview = repo.filtered(candidate) rview = repo.filtered(candidate)
if cache.validfor(rview): if cache.validfor(rview):
repo._branchcaches[candidate] = cache self._per_filter[candidate] = cache
cache.write(rview) cache.write(rview)
break return
def clear(self):
self._per_filter.clear()
class branchcache(dict): class branchcache(dict):
"""A dict like object that hold branches heads cache. """A dict like object that hold branches heads cache.
This cache is used to avoid costly computations to determine all the This cache is used to avoid costly computations to determine all the
branch heads of a repo. branch heads of a repo.
The cache is serialized on disk in the following format: The cache is serialized on disk in the following format:

mercurial/localrepo.py

self.ui.configbool('devel', 'check-locks')): self.ui.configbool('devel', 'check-locks')):
if util.safehasattr(self.svfs, 'vfs'): # this is filtervfs if util.safehasattr(self.svfs, 'vfs'): # this is filtervfs
self.svfs.vfs.audit = self._getsvfsward(self.svfs.vfs.audit) self.svfs.vfs.audit = self._getsvfsward(self.svfs.vfs.audit)
else: # standard vfs else: # standard vfs
self.svfs.audit = self._getsvfsward(self.svfs.audit) self.svfs.audit = self._getsvfsward(self.svfs.audit)
self._dirstatevalidatewarned = False self._dirstatevalidatewarned = False
self._branchcaches = {} self._branchcaches = branchmap.BranchMapCache()
self._revbranchcache = None self._revbranchcache = None
self._filterpats = {} self._filterpats = {}
self._datafilters = {} self._datafilters = {}
self._transref = self._lockref = self._wlockref = None self._transref = self._lockref = self._wlockref = None
# A cache for various files under .hg/ that tracks file changes, # A cache for various files under .hg/ that tracks file changes,
# (used by the filecache decorator) # (used by the filecache decorator)
# #
def nodebookmarks(self, node): def nodebookmarks(self, node):
"""return the list of bookmarks pointing to the specified node""" """return the list of bookmarks pointing to the specified node"""
return self._bookmarks.names(node) return self._bookmarks.names(node)
def branchmap(self): def branchmap(self):
'''returns a dictionary {branch: [branchheads]} with branchheads '''returns a dictionary {branch: [branchheads]} with branchheads
ordered by increasing revision number''' ordered by increasing revision number'''
branchmap.updatecache(self) return self._branchcaches[self]
return self._branchcaches[self.filtername]
@unfilteredmethod @unfilteredmethod
def revbranchcache(self): def revbranchcache(self):
if not self._revbranchcache: if not self._revbranchcache:
self._revbranchcache = branchmap.revbranchcache(self.unfiltered()) self._revbranchcache = branchmap.revbranchcache(self.unfiltered())
return self._revbranchcache return self._revbranchcache
def branchtip(self, branch, ignoremissing=False): def branchtip(self, branch, ignoremissing=False):
up-to-date data. Even the ones usually loaded more lazily. up-to-date data. Even the ones usually loaded more lazily.
""" """
if tr is not None and tr.hookargs.get('source') == 'strip': if tr is not None and tr.hookargs.get('source') == 'strip':
# During strip, many caches are invalid but # During strip, many caches are invalid but
# later call to `destroyed` will refresh them. # later call to `destroyed` will refresh them.
return return
if tr is None or tr.changes['origrepolen'] < len(self): if tr is None or tr.changes['origrepolen'] < len(self):
# updating the unfiltered branchmap should refresh all the others, # accessing the 'ser ved' branchmap should refresh all the others,
self.ui.debug('updating the branch cache\n') self.ui.debug('updating the branch cache\n')
branchmap.updatecache(self.filtered('served')) self.filtered('served').branchmap()
martinvonzUnsubmitted
Not Done

Hmm, it's much less clear now that this updates the cache. At the very least, it deserves a comment. Is the updatecache() call from __getitem__() necessary for your later patches? Sorry, I didn't quite follow.

martinvonz: Hmm, it's much less clear now that this updates the cache. At the very least, it deserves a…
mjpietersAuthorUnsubmitted
Not Done

Yes, the updating will change in a later, as yet to be submitted patch.

But why should it be clear here that there is a cache that is kept up-to-date on access to the branchmap? That's a responsibility of the cache, not of whatever accesses a branchmap. The cache is an implementation detail of branchmap, and should really not bleed out into code that merely consumes the map.

mjpieters: Yes, the updating will change in a later, as yet to be submitted patch. But why should it be…
martinvonzUnsubmitted
Not Done

Fair enough, it doesn't have to be clear that it updates the cache, but it has to be clear that it does *something*. It currently looks like a statement that can be removed, but I assume it cannot.

martinvonz: Fair enough, it doesn't have to be clear that it updates the cache, but it has to be clear that…
if full: if full:
rbc = self.revbranchcache() rbc = self.revbranchcache()
for r in self.changelog: for r in self.changelog:
rbc.branchinfo(r) rbc.branchinfo(r)
rbc.write() rbc.write()
# ensure the working copy parents are in the manifestfulltextcache # ensure the working copy parents are in the manifestfulltextcache
for ctx in self['.'].parents(): for ctx in self['.'].parents():
ctx.manifest() # accessing the manifest is enough ctx.manifest() # accessing the manifest is enough
def invalidatecaches(self): def invalidatecaches(self):
if r'_tagscache' in vars(self): if r'_tagscache' in vars(self):
# can't use delattr on proxy # can't use delattr on proxy
del self.__dict__[r'_tagscache'] del self.__dict__[r'_tagscache']
self.unfiltered()._branchcaches.clear() self._branchcaches.clear()
self.invalidatevolatilesets() self.invalidatevolatilesets()
self._sparsesignaturecache.clear() self._sparsesignaturecache.clear()
def invalidatevolatilesets(self): def invalidatevolatilesets(self):
self.filteredrevcache.clear() self.filteredrevcache.clear()
obsolete.clearobscaches(self) obsolete.clearobscaches(self)
def invalidatedirstate(self): def invalidatedirstate(self):

mercurial/statichttprepo.py

# statichttprepo.py - simple http repository class for mercurial # statichttprepo.py - simple http repository class for mercurial
# #
# This provides read-only repo access to repositories exported via static http # This provides read-only repo access to repositories exported via static http
# #
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
from .i18n import _ from .i18n import _
from . import ( from . import (
branchmap,
changelog, changelog,
error, error,
localrepo, localrepo,
manifest, manifest,
namespaces, namespaces,
pathutil, pathutil,
url, url,
util, util,
self.requirements = requirements self.requirements = requirements
rootmanifest = manifest.manifestrevlog(self.svfs) rootmanifest = manifest.manifestrevlog(self.svfs)
self.manifestlog = manifest.manifestlog(self.svfs, self, rootmanifest, self.manifestlog = manifest.manifestlog(self.svfs, self, rootmanifest,
self.narrowmatch()) self.narrowmatch())
self.changelog = changelog.changelog(self.svfs) self.changelog = changelog.changelog(self.svfs)
self._tags = None self._tags = None
self.nodetagscache = None self.nodetagscache = None
self._branchcaches = {} self._branchcaches = branchmap.BranchMapCache()
self._revbranchcache = None self._revbranchcache = None
self.encodepats = None self.encodepats = None
self.decodepats = None self.decodepats = None
self._transref = None self._transref = None
def _restrictcapabilities(self, caps): def _restrictcapabilities(self, caps):
caps = super(statichttprepository, self)._restrictcapabilities(caps) caps = super(statichttprepository, self)._restrictcapabilities(caps)
return caps.difference(["pushkey"]) return caps.difference(["pushkey"])

mercurial/streamclone.py

# streamclone.py - producing and consuming streaming repository data # streamclone.py - producing and consuming streaming repository data
# #
# Copyright 2015 Gregory Szorc <gregory.szorc@gmail.com> # Copyright 2015 Gregory Szorc <gregory.szorc@gmail.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import contextlib import contextlib
import os import os
import struct import struct
from .i18n import _ from .i18n import _
from . import ( from . import (
branchmap, branchmap,
martinvonzUnsubmitted
Done

test-check-pyflakes.t says that this is now unused

martinvonz: `test-check-pyflakes.t` says that this is now unused
cacheutil, cacheutil,
error, error,
narrowspec, narrowspec,
phases, phases,
pycompat, pycompat,
repository, repository,
store, store,
util, util,
# requirements from the streamed-in repository # requirements from the streamed-in repository
repo.requirements = requirements | ( repo.requirements = requirements | (
repo.requirements - repo.supportedformats) repo.requirements - repo.supportedformats)
repo.svfs.options = localrepo.resolvestorevfsoptions( repo.svfs.options = localrepo.resolvestorevfsoptions(
repo.ui, repo.requirements, repo.features) repo.ui, repo.requirements, repo.features)
repo._writerequirements() repo._writerequirements()
if rbranchmap: if rbranchmap:
branchmap.replacecache(repo, rbranchmap) repo._branchcaches.replace(repo, rbranchmap)
repo.invalidate() repo.invalidate()
def allowservergeneration(repo): def allowservergeneration(repo):
"""Whether streaming clones are allowed from the server.""" """Whether streaming clones are allowed from the server."""
if repository.REPO_FEATURE_STREAM_CLONE not in repo.features: if repository.REPO_FEATURE_STREAM_CLONE not in repo.features:
return False return False