D5638: branchmap: encapsulate cache updating in the map itself
This revision was automatically updated to reflect the committed changes. Closed by commit rHG328ca3b9e545: branchmap: encapsulate cache updating in the map itself (authored by mjpieters, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5638?vs=13543&id=13914 REVISION DETAIL https://phab.mercurial-scm.org/D5638 AFFECTED FILES contrib/perf.py mercurial/branchmap.py mercurial/localrepo.py mercurial/statichttprepo.py mercurial/streamclone.py CHANGE DETAILS diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py --- a/mercurial/streamclone.py +++ b/mercurial/streamclone.py @@ -13,7 +13,6 @@ from .i18n import _ from . import ( -branchmap, cacheutil, error, narrowspec, @@ -174,7 +173,7 @@ repo._writerequirements() if rbranchmap: -branchmap.replacecache(repo, rbranchmap) +repo._branchcaches.replace(repo, rbranchmap) repo.invalidate() diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py --- a/mercurial/statichttprepo.py +++ b/mercurial/statichttprepo.py @@ -13,6 +13,7 @@ from .i18n import _ from . import ( +branchmap, changelog, error, localrepo, @@ -193,7 +194,7 @@ self.changelog = changelog.changelog(self.svfs) self._tags = None self.nodetagscache = None -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self.encodepats = None self.decodepats = None diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -992,7 +992,7 @@ self._dirstatevalidatewarned = False -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self._filterpats = {} self._datafilters = {} @@ -1520,8 +1520,7 @@ def branchmap(self): '''returns a dictionary {branch: [branchheads]} with branchheads ordered by increasing revision number''' -branchmap.updatecache(self) -return self._branchcaches[self.filtername] +return self._branchcaches[self] @unfilteredmethod def revbranchcache(self): @@ -2073,9 +2072,9 @@ return 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') -branchmap.updatecache(self.filtered('served')) +self.filtered('served').branchmap() if full: rbc = self.revbranchcache() @@ -2093,7 +2092,7 @@ # can't use delattr on proxy del self.__dict__[r'_tagscache'] -self.unfiltered()._branchcaches.clear() +self._branchcaches.clear() self.invalidatevolatilesets() self._sparsesignaturecache.clear() diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py --- a/mercurial/branchmap.py +++ b/mercurial/branchmap.py @@ -43,75 +43,89 @@ 'served': 'immutable', 'immutable': 'base'} -def updatecache(repo): -"""Update the cache for the given filtered view on a repository""" -# 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 -# is stale. + +class BranchMapCache(object): +"""Cache mapping""" +def __init__(self): +self._per_filter = {} -cl = repo.changelog -filtername = repo.filtername -bcache = repo._branchcaches.get(filtername) -if bcache is None or not bcache.validfor(repo): -# cache object missing or cache object stale? Read from disk -bcache = branchcache.fromfile(repo) +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""" +# 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 +# is stale. -revs = [] -if bcache is None: -# no (fresh) cache available anymore, perhaps we can re-use -# the cache for a subset, then extend that to add info on missing -# revisions. -subsetname = subsettable.get(filtername) -if subsetname is not None: -subset = repo.filtered(subsetname) -bcache = subset.branchmap().copy() -extrarevs = subset.changelog.filteredrevs - cl.filteredrevs -revs.extend(r for r in extrarevs if r <= bcache.tiprev) -else: -# nothing to fall back on, start empty. -
D5638: branchmap: encapsulate cache updating in the map itself
martinvonz added a comment. In https://phab.mercurial-scm.org/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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5638 To: mjpieters, #hg-reviewers Cc: pulkit, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5638: branchmap: encapsulate cache updating in the map itself
pulkit added a comment. This looks good to me. @martinvonz do you have any concerns? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5638 To: mjpieters, #hg-reviewers Cc: pulkit, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5638: branchmap: encapsulate cache updating in the map itself
mjpieters updated this revision to Diff 13543. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5638?vs=13502&id=13543 REVISION DETAIL https://phab.mercurial-scm.org/D5638 AFFECTED FILES contrib/perf.py mercurial/branchmap.py mercurial/localrepo.py mercurial/statichttprepo.py mercurial/streamclone.py CHANGE DETAILS diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py --- a/mercurial/streamclone.py +++ b/mercurial/streamclone.py @@ -13,7 +13,6 @@ from .i18n import _ from . import ( -branchmap, cacheutil, error, narrowspec, @@ -174,7 +173,7 @@ repo._writerequirements() if rbranchmap: -branchmap.replacecache(repo, rbranchmap) +repo._branchcaches.replace(repo, rbranchmap) repo.invalidate() diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py --- a/mercurial/statichttprepo.py +++ b/mercurial/statichttprepo.py @@ -13,6 +13,7 @@ from .i18n import _ from . import ( +branchmap, changelog, error, localrepo, @@ -192,7 +193,7 @@ self.changelog = changelog.changelog(self.svfs) self._tags = None self.nodetagscache = None -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self.encodepats = None self.decodepats = None diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -992,7 +992,7 @@ self._dirstatevalidatewarned = False -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self._filterpats = {} self._datafilters = {} @@ -1520,8 +1520,7 @@ def branchmap(self): '''returns a dictionary {branch: [branchheads]} with branchheads ordered by increasing revision number''' -branchmap.updatecache(self) -return self._branchcaches[self.filtername] +return self._branchcaches[self] @unfilteredmethod def revbranchcache(self): @@ -2073,9 +2072,9 @@ return 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') -branchmap.updatecache(self.filtered('served')) +self.filtered('served').branchmap() if full: rbc = self.revbranchcache() @@ -2093,7 +2092,7 @@ # can't use delattr on proxy del self.__dict__[r'_tagscache'] -self.unfiltered()._branchcaches.clear() +self._branchcaches.clear() self.invalidatevolatilesets() self._sparsesignaturecache.clear() diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py --- a/mercurial/branchmap.py +++ b/mercurial/branchmap.py @@ -43,75 +43,89 @@ 'served': 'immutable', 'immutable': 'base'} -def updatecache(repo): -"""Update the cache for the given filtered view on a repository""" -# 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 -# is stale. + +class BranchMapCache(object): +"""Cache mapping""" +def __init__(self): +self._per_filter = {} -cl = repo.changelog -filtername = repo.filtername -bcache = repo._branchcaches.get(filtername) -if bcache is None or not bcache.validfor(repo): -# cache object missing or cache object stale? Read from disk -bcache = branchcache.fromfile(repo) +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""" +# 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 +# is stale. -revs = [] -if bcache is None: -# no (fresh) cache available anymore, perhaps we can re-use -# the cache for a subset, then extend that to add info on missing -# revisions. -subsetname = subsettable.get(filtername) -if subsetname is not None: -subset = repo.filtered(subsetname) -bcache = subset.branchmap().copy() -extrarevs = subset.changelog.filteredrevs - cl.filteredrevs -revs.extend(r for r in extrarevs if r <= bcache.tiprev) -else: -# nothing to fall back on, start empty. -bcache = branchcache() +cl = repo.changelog +filtername = repo.filtername +bcache = self._per_filter.get(filtername) +if bcach
D5638: branchmap: encapsulate cache updating in the map itself
martinvonz added inline comments. INLINE COMMENTS > mjpieters wrote in 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. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5638 To: mjpieters, #hg-reviewers Cc: martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5638: branchmap: encapsulate cache updating in the map itself
mjpieters added inline comments. INLINE COMMENTS > martinvonz wrote in perf.py:2299 > Same here: I think this needs to be made compatible with both versions > (before and after this patch) I'll wait for confirmation; see the other patch. > martinvonz wrote in 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. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5638 To: mjpieters, #hg-reviewers Cc: martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5638: branchmap: encapsulate cache updating in the map itself
mjpieters updated this revision to Diff 13502. mjpieters marked an inline comment as done. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5638?vs=1&id=13502 REVISION DETAIL https://phab.mercurial-scm.org/D5638 AFFECTED FILES contrib/perf.py mercurial/branchmap.py mercurial/localrepo.py mercurial/statichttprepo.py mercurial/streamclone.py CHANGE DETAILS diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py --- a/mercurial/streamclone.py +++ b/mercurial/streamclone.py @@ -13,7 +13,6 @@ from .i18n import _ from . import ( -branchmap, cacheutil, error, narrowspec, @@ -174,7 +173,7 @@ repo._writerequirements() if rbranchmap: -branchmap.replacecache(repo, rbranchmap) +repo._branchcaches.replace(repo, rbranchmap) repo.invalidate() diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py --- a/mercurial/statichttprepo.py +++ b/mercurial/statichttprepo.py @@ -13,6 +13,7 @@ from .i18n import _ from . import ( +branchmap, changelog, error, localrepo, @@ -192,7 +193,7 @@ self.changelog = changelog.changelog(self.svfs) self._tags = None self.nodetagscache = None -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self.encodepats = None self.decodepats = None diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -992,7 +992,7 @@ self._dirstatevalidatewarned = False -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self._filterpats = {} self._datafilters = {} @@ -1520,8 +1520,7 @@ def branchmap(self): '''returns a dictionary {branch: [branchheads]} with branchheads ordered by increasing revision number''' -branchmap.updatecache(self) -return self._branchcaches[self.filtername] +return self._branchcaches[self] @unfilteredmethod def revbranchcache(self): @@ -2073,9 +2072,9 @@ return 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') -branchmap.updatecache(self.filtered('served')) +self.filtered('served').branchmap() if full: rbc = self.revbranchcache() @@ -2093,7 +2092,7 @@ # can't use delattr on proxy del self.__dict__[r'_tagscache'] -self.unfiltered()._branchcaches.clear() +self._branchcaches.clear() self.invalidatevolatilesets() self._sparsesignaturecache.clear() diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py --- a/mercurial/branchmap.py +++ b/mercurial/branchmap.py @@ -43,75 +43,89 @@ 'served': 'immutable', 'immutable': 'base'} -def updatecache(repo): -"""Update the cache for the given filtered view on a repository""" -# 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 -# is stale. + +class BranchMapCache(object): +"""Cache mapping""" +def __init__(self): +self._per_filter = {} -cl = repo.changelog -filtername = repo.filtername -bcache = repo._branchcaches.get(filtername) -if bcache is None or not bcache.validfor(repo): -# cache object missing or cache object stale? Read from disk -bcache = branchcache.fromfile(repo) +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""" +# 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 +# is stale. -revs = [] -if bcache is None: -# no (fresh) cache available anymore, perhaps we can re-use -# the cache for a subset, then extend that to add info on missing -# revisions. -subsetname = subsettable.get(filtername) -if subsetname is not None: -subset = repo.filtered(subsetname) -bcache = subset.branchmap().copy() -extrarevs = subset.changelog.filteredrevs - cl.filteredrevs -revs.extend(r for r in extrarevs if r <= bcache.tiprev) -else: -# nothing to fall back on, start empty. -bcache = branchcache() +cl = repo.changelog +filtername = repo.filtername +bcache = self._
D5638: branchmap: encapsulate cache updating in the map itself
martinvonz added inline comments. INLINE COMMENTS > perf.py:2299 > else: > -view._branchcaches.pop(filtername, None) > +view._branchcaches._per_filter.pop(filtername, None) > view.branchmap() Same here: I think this needs to be made compatible with both versions (before and after this patch) > localrepo.py:2077 > self.ui.debug('updating the branch cache\n') > -branchmap.updatecache(self.filtered('served')) > +self.filtered('served').branchmap() > 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. > streamclone.py:16 > from . import ( > branchmap, > cacheutil, `test-check-pyflakes.t` says that this is now unused REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5638 To: mjpieters, #hg-reviewers Cc: martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5638: branchmap: encapsulate cache updating in the map itself
mjpieters created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5638 AFFECTED FILES contrib/perf.py mercurial/branchmap.py mercurial/localrepo.py mercurial/statichttprepo.py mercurial/streamclone.py CHANGE DETAILS diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py --- a/mercurial/streamclone.py +++ b/mercurial/streamclone.py @@ -174,7 +174,7 @@ repo._writerequirements() if rbranchmap: -branchmap.replacecache(repo, rbranchmap) +repo._branchcaches.replace(repo, rbranchmap) repo.invalidate() diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py --- a/mercurial/statichttprepo.py +++ b/mercurial/statichttprepo.py @@ -13,6 +13,7 @@ from .i18n import _ from . import ( +branchmap, changelog, error, localrepo, @@ -192,7 +193,7 @@ self.changelog = changelog.changelog(self.svfs) self._tags = None self.nodetagscache = None -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self.encodepats = None self.decodepats = None diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -991,7 +991,7 @@ self._dirstatevalidatewarned = False -self._branchcaches = {} +self._branchcaches = branchmap.BranchMapCache() self._revbranchcache = None self._filterpats = {} self._datafilters = {} @@ -1519,8 +1519,7 @@ def branchmap(self): '''returns a dictionary {branch: [branchheads]} with branchheads ordered by increasing revision number''' -branchmap.updatecache(self) -return self._branchcaches[self.filtername] +return self._branchcaches[self] @unfilteredmethod def revbranchcache(self): @@ -2073,9 +2072,9 @@ return 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') -branchmap.updatecache(self.filtered('served')) +self.filtered('served').branchmap() if full: rbc = self.revbranchcache() @@ -2093,7 +2092,7 @@ # can't use delattr on proxy del self.__dict__[r'_tagscache'] -self.unfiltered()._branchcaches.clear() +self._branchcaches.clear() self.invalidatevolatilesets() self._sparsesignaturecache.clear() diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py --- a/mercurial/branchmap.py +++ b/mercurial/branchmap.py @@ -43,75 +43,89 @@ 'served': 'immutable', 'immutable': 'base'} -def updatecache(repo): -"""Update the cache for the given filtered view on a repository""" -# 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 -# is stale. + +class BranchMapCache(object): +"""Cache mapping""" +def __init__(self): +self._per_filter = {} -cl = repo.changelog -filtername = repo.filtername -bcache = repo._branchcaches.get(filtername) -if bcache is None or not bcache.validfor(repo): -# cache object missing or cache object stale? Read from disk -bcache = branchcache.fromfile(repo) +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""" +# 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 +# is stale. -revs = [] -if bcache is None: -# no (fresh) cache available anymore, perhaps we can re-use -# the cache for a subset, then extend that to add info on missing -# revisions. -subsetname = subsettable.get(filtername) -if subsetname is not None: -subset = repo.filtered(subsetname) -bcache = subset.branchmap().copy() -extrarevs = subset.changelog.filteredrevs - cl.filteredrevs -revs.extend(r for r in extrarevs if r <= bcache.tiprev) -else: -# nothing to fall back on, start empty. -bcache = branchcache() +cl = repo.changelog +