Loading the filtered repo can be slow if there are many obsmarkers and
it's not needed in the plain hg status case. This saves about 60ms
in my hg repo (hg st goes from 0.244s to 0.179s) and about 200ms in
a repo I use at work (where we have an extension that hooks into
repoview.pinnedrevs()).
Details
Details
- Reviewers
durin42 - Group Reviewers
hg-reviewers
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
Comment Actions
To reviewers: I only care about this patch (and its parent is needed for this to pass tests). Feel free to drop any other patches you don't like.
Comment Actions
I have a couple of questions:
- Can we get performance number with about without this patch ? (to gauge the relative gain)
- This could cause issue to extensions, can we abtract the unfiltering though a function so that extension can rewrap it ?
Comment Actions
I am +1 on this idea. I see @marmoute has some concerns around extensions wrapping, once they are addressed, I will push it once the freeze ends.
Comment Actions
Will get that later (I have ~2 weeks before the end of freeze anyway :))
- This could cause issue to extensions, can we abtract the unfiltering though a function so that extension can rewrap it ?
To make sure I address it properly, what's the issue(s) you're thinking of?
| mercurial/commands.py | ||
|---|---|---|
| 6808 | Are you using the evolve extension. If so you should -already- not be loading obsmarkers (but there are other filtering associated cost). If you are using evolve and actually loading obsmarkers for this operation, we have a regression somewhere to fix first. | |
Comment Actions
Another question is " Why are we loading the changelog if we don't need it ?". If the code were not accessing the changelog, we could not pay the filtering code (and save other parsing cost).
Comment Actions
We load the changelog when we do repo['.']. I had added a hack (https://www.mercurial-scm.org/repo/hg/file/4565a0afc289/mercurial/localrepo.py#l1539) to try to avoid that, but it was no longer effective. I think using the unfiltered repo is a better solution anyway.
Comment Actions
@marmoute, could you explain what the issue is? And what function would I extract to help solve it?
Comment Actions
The comment about maybe affecting extensions made me think of df463ca0adef. I never figured out what the issue was.
Comment Actions
Imagine you are an extension writer adding extra information on status (eg: tell the user if his stack is behind and he needs to rebase). If you get an unfiltered repository, any operation you run (eg: a revset to check the graph, will go horribly wrong). So we need need at minimum an easy way to disable the unfiltering if an extension needs to do it.
If the performance gain are clear, using an unfiltered repository seems like a reasonable gain/risk move. However I would prefer this to remain temporary. Keeping the repository filtering aligned to the current semantic is important to avoid surprise and obscure bug in the future. We need an overall effort to reduce the filtering impact anyway (independantly from this series).
In the case here, I wonder if we have and easy way to prevent the filtering computation kick in, as it is clearly unnecessary.
- can you share a profile of what takes time with the filtering version
- Do you know which part is triggering the filtering ?
Comment Actions
I suspect that's because largefiles creates a repo subclass and that subclass does not have the __getattr__ and __setattr__ overrides that repoview does, so setting lfstatusor _largefilesenabled on that subclass won't propagate it to the parent.
I'll get back to you about the other things later, but I think the following profile answers these last questions:
| 100.0% 0.19s dispatch.py: _callcatch line 36: dispatch.run()
| 100.0% 0.19s scmutil.py: callcatch line 433: return scmutil.callcatch(ui...
| 100.0% 0.19s dispatch.py: _runcatchfunc line 177: return func()
| 100.0% 0.19s hg.py: repository line 414: return _dispatch(req)
\ 52.6% 0.10s localrepo.py: status line 6831: opts.get(b'subrepos'),
\ 31.6% 0.06s localrepo.py: __getitem__ line 3248: return self[node1].status(
| 31.6% 0.06s repoview.py: changelog line 1547: rev = self.changelog.rev(ch...
| 31.6% 0.06s repoview.py: filterrevs line 278: revs = filterrevs(unfi, sel...
| 31.6% 0.06s repoview.py: computehidden line 217: repo.filteredrevcache[filte...
\ 15.8% 0.03s repoview.py: hideablerevsline 87: hidden = hideablerevs(repo)
| 15.8% 0.03s obsolete.py: getrevs line 39: obsoletes = obsolete.getrev...
| 15.8% 0.03s obscache.py: <lambda> line 905: repo.obsstore.caches[name] ...
| 15.8% 0.03s obscache.py: _computeobsoletesetline 469: wrapped = lambda repo: _com...
| 10.5% 0.02s phases.py: getrevset line 434: notpublic = repo._phasecach...
\ 5.3% 0.01s scmutil.py: __get__ line 106: return super(_basefilecache...
| 5.3% 0.01s localrepo.py: _phasecacheline 1636: entry.obj = self.func(obj)
| 5.3% 0.01s phases.py: __init__line 1432: return phases.phasecache(se...
| 5.3% 0.01s phases.py: _readrootsline 235: self.phaseroots, self.dirty...
\ 5.3% 0.01s phases.py: loadphaserevsline 243: self.loadphaserevs(repo) #...
| 5.3% 0.01s phases.py: _getphaserevsnativeline 325: res = self._getphaserevsnat...
| 5.3% 0.01s changelog.py: rev line 301: pycompat.maplist(repo.chang...
\ 10.5% 0.02s repoview.py: pinnedrevs line 89: hidden = set(hidden - pinne...
\ 5.3% 0.01s repoview.py: <genexpr> line 58: pinned.update(rev(t[0]) for...
\ 5.3% 0.01s tags.py: readlocaltagsline 55: tagsmod.readlocaltags(repo....
| 5.3% 0.01s tags.py: _readtagsline 255: ui, repo, data.splitlines()...
| 5.3% 0.01s tags.py: _readtaghistline 337: ui, repo, lines, fn, recode...
\ 5.3% 0.01s repoview.py: _revealancestorsline 96: _revealancestors(pfunc, hid...
| 5.3% 0.01s smartset.py: _iterfilterline 73: stack = list(revs)
| 5.3% 0.01s smartset.py: __init__ line 467: l = baseset(r for r in self)
| 5.3% 0.01s smartset.py: <genexpr> line 242: data = list(data)
| 5.3% 0.01s smartset.py: _iterfilterline 467: l = baseset(r for r in self)
| 5.3% 0.01s smartset.py: <lambda> line 421: if cond(x):
\ 21.1% 0.04s context.py: status line 3249: node2, match, ignored, clea...
| 21.1% 0.04s localrepo.py: narrowmatch line 424: ctx1, r, match, listignored...
| 21.1% 0.04s context.py: _dirstatestatusline 1878: s = self._dirstatestatus(ma...
| 21.1% 0.04s dirstate.py: status line 1788: match, subrepos, ignored=ig...
| 21.1% 0.04s dirstate.py: walk line 1117: self.walk(match, subrepos, ...
\ 15.8% 0.03s dirstate.py: traverse line 1018: traverse([d], alreadynormed)
| 5.3% 0.01s dirstate.py: __contains__line 1001: if nf in dmap:
\ 5.3% 0.01s scmutil.py: __get__ line 911: ignore = self._ignore
| 5.3% 0.01s dirstate.py: _ignore line 1636: entry.obj = self.func(obj)
| 5.3% 0.01s match.py: match line 167: return matchmod.match(self....
| 5.3% 0.01s match.py: _buildkindpatsmatcherline 277: badfn=None,
| 5.3% 0.01s match.py: __init__ line 127: m = matchercls(root, kindpa...
| 5.3% 0.01s match.py: _buildmatchline 673: self._pats, self.matchfn = ...
| 5.3% 0.01s match.py: _buildregexmatchline 1368: regex, mf = _buildregexmatc...
| 5.3% 0.01s match.py: _rematcherline 1417: matcher = _rematcher(fullre...
| 5.3% 0.01s util.py: compile line 52: m = util.re.compile(regex)
| 5.3% 0.01s re.py: compile line 2168: return remod.compile(pat, f...
| 5.3% 0.01s re.py: _compile line 194: return _compile(pattern, fl...
| 5.3% 0.01s sre_compile.py: compile line 249: p = sre_compile.compile(pat...
| 5.3% 0.01s sre_parse.py: parse line 572: p = sre_parse.parse(p, flags)
| 5.3% 0.01s sre_parse.py: _parse_subline 735: p = _parse_sub(source, patt...
| 5.3% 0.01s sre_parse.py: _parse line 343: itemsappend(_parse(source, ...
| 5.3% 0.01s sre_parse.py: append line 448: subpatternappend((LITERAL, ...
\ 21.1% 0.04s ui.py: pager line 6843: ui.pager(b'status')
| 21.1% 0.04s ui.py: _runpager line 1289: if self._runpager(pagercmd,...
| 21.1% 0.04s subprocess.py: __init__ line 1347: env=procutil.tonativeenv(pr...
| 21.1% 0.04s subprocess.py: _execute_child line 394: errread, errwrite)
| 21.1% 0.04s subprocess.py: _eintr_retry_callline 1023: data = _eintr_retry_call(os...
\ 15.8% 0.03s extensions.py: load line 294: load(ui, name, path, loadin...
| 15.8% 0.03s extensions.py: _importext line 211: mod = _importext(name, path...
\ 10.5% 0.02s extensions.py: _importh line 127: mod = _importh(b"hgext3rd.%...
| 10.5% 0.02s demandimportpy2.py: _demandimportline 107: mod = __import__(pycompat.s...
| 10.5% 0.02s demandimportpy2.py: _hgextimportline 181: return _hgextimport(_origim...
| 10.5% 0.02s __init__.py: <module> line 44: return importfunc(name, glo...
\ 5.3% 0.01s demandimportpy2.py: __getattr__line 344: eh.merge(obsexchange.eh)
| 5.3% 0.01s demandimportpy2.py: _loadline 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s obsexchange.py: <module>line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: __getattr__line 36: eh.merge(obsdiscovery.eh)
| 5.3% 0.01s demandimportpy2.py: _loadline 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s obsdiscovery.py: <module>line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: __getattr__line 332: class _obshashcache(obscach...
| 5.3% 0.01s demandimportpy2.py: _loadline 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s obscache.py: <module>line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: __getattr__line 28: obsstorefilecache = localre...
| 5.3% 0.01s demandimportpy2.py: _loadline 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s localrepo.py: <module>line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: _demandimportline 30: from . import (
| 5.3% 0.01s demandimportpy2.py: processfromitemline 278: processfromitem(mod, x)
\ 5.3% 0.01s cmdutil.py: findcmd line 332: aliases, entry = cmdutil.fi...
| 5.3% 0.01s cmdutil.py: findpossibleline 859: choice, allcmds = findpossi...
\ 5.3% 0.01s extensions.py: _importh line 122: mod = _importh(b"hgext.%s" ...
| 5.3% 0.01s demandimportpy2.py: _demandimportline 107: mod = __import__(pycompat.s...
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 181: return _hgextimport(_origim...
| 5.3% 0.01s phabricator.py: <module> line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: __getattr__line 51: help.CATEGORY_ORDER.insert(
| 5.3% 0.01s demandimportpy2.py: _load line 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s help.py: <module> line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: __getattr__line 592: addtopicsymbols(b'filesets'...
| 5.3% 0.01s demandimportpy2.py: _load line 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s fileset.py: <module> line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: _demandimportline 15: from . import (
| 5.3% 0.01s demandimportpy2.py: processfromitemline 278: processfromitem(mod, x)
\ 5.3% 0.01s scmutil.py: revpair line 6782: ctx1, ctx2 = scmutil.revpai...
| 5.3% 0.01s localrepo.py: __getitem__ line 732: return repo[b'.'], repo[None]
| 5.3% 0.01s dirstate.py: p1 line 1542: node = self.dirstate.p1()
| 5.3% 0.01s localrepo.py: _dirstatevalidateline 293: return self._validate(self....
| 5.3% 0.01s localrepo.py: __get__ line 1460: self.changelog.rev(node)
| 5.3% 0.01s scmutil.py: __get__ line 106: return super(_basefilecache...
| 5.3% 0.01s localrepo.py: changelog line 1636: entry.obj = self.func(obj)
| 5.3% 0.01s store.py: changelog line 1440: return self.store.changelog...
| 5.3% 0.01s changelog.py: __init__ line 424: return changelog.changelog(...
| 5.3% 0.01s demandimportpy2.py: _load line 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s changelog.py: <module> line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: __getattr__ line 354: class changelog(revlog.revl...
| 5.3% 0.01s demandimportpy2.py: _load line 157: self._load()
| 5.3% 0.01s demandimportpy2.py: _hgextimportline 97: _origimport, head, globals,...
| 5.3% 0.01s revlog.py: <module> line 44: return importfunc(name, glo...
| 5.3% 0.01s demandimportpy2.py: _demandimportline 39: from .revlogutils.constants...
| 5.3% 0.01s demandimportpy2.py: processfromitemline 278: processfromitem(mod, x)
\ 5.3% 0.01s fancyopts.py: fancyopts line 922: args = fancyopts.fancyopts(...
---
Sample count: 103
Total time: 0.180000 seconds (0.190000 wall)Comment Actions
I've extracted a function for deciding whether to use an unfiltered repo, so hopefully this can get queued now.
Comment Actions
I took more time to look at your series and at the bigger picture. My general position is still that passing the wrong filter level to high level function is hacking and will be a significant source of bugs.
For example, hg status taks a "PATTERN" argument, that pattern can be fileset, that fileset can contains revset. PAssing the wrong filtering will result in wrongdoing of that revset.
However it might be a good idea to do this hack if:
- the performance gain is significant,
- the hack is temporary,
- there is no reasonable alternative available short terms.
I took some time to poke at the issues, and I managed to get all 4 commands listed in this series with a first throw of abotu 12 changesets. The idea is to recognise pattern that will not be filtered and to have lower level logic skip around the filtering in this case. So the higher level logic is still filtered with the right semantic. One can have a look at the very first throw here: https://dev.heptapod.net/octobus/mercurial-devel/commits/topic/default/filter-trigger
It is getting late so I will stop here for tonight. However, the prototype looks promissing enough. I can probably have a clean/consolidate version by the end of the week.
Comment Actions
I agree that it's a hack.
For example, hg status taks a "PATTERN" argument, that pattern can be fileset, that fileset can contains revset. PAssing the wrong filtering will result in wrongdoing of that revset.
However it might be a good idea to do this hack if:
- the performance gain is significant,
- the hack is temporary,
- there is no reasonable alternative available short terms.
I took some time to poke at the issues, and I managed to get all 4 commands listed in this series with a first throw of abotu 12 changesets. The idea is to recognise pattern that will not be filtered and to have lower level logic skip around the filtering in this case. So the higher level logic is still filtered with the right semantic. One can have a look at the very first throw here: https://dev.heptapod.net/octobus/mercurial-devel/commits/topic/default/filter-trigger
I did something similar (but much smaller in scope) a long time ago: https://www.mercurial-scm.org/repo/hg/rev/a9b61dbdb827. That hack has since become ineffective, and I think some of your commits aim at restoring its effect. I felt that that approach was also fragile since quite subtle code changes can make it stop working (as has already happened with my original hack). So I'm not convinced that is the right approach either, but I'm fine with taking that series instead if you clean it up (I do see the risk of bugs with my series).
Relatedly, I would really like to have the filtering applied only when walking towards children in the changelog (this is something that Durham has mentioned before), so no filtering happens even if you do hg status -r <some hidden commit> or similar. But that's a much larger change, of course.
It is getting late so I will stop here for tonight. However, the prototype looks promissing enough. I can probably have a clean/consolidate version by the end of the week.
Comment Actions
I think a consolidated approach will work reasonably well in most simpe case. However, yes, the approach is "fragile" since any other code directly touching repo.changelog would trigger the filtering. (We could probably improve that too).
Since I am about to add a filtered repository to the benchmarks, we should be able to detect regression here. That's not ideal… but eventually we will also speed up the filtering itself.
Relatedly, I would really like to have the filtering applied only when walking towards children in the changelog (this is something that Durham has mentioned before),
I think a generalisation of my current approach is close to that. If you walk parent from a revision that you know to exists, you don't need to check filtering.
so no filtering happens even if you do hg status -r <some hidden commit> or similar. But that's a much larger change, of course.
It is getting late so I will stop here for tonight. However, the prototype looks promissing enough. I can probably have a clean/consolidate version by the end of the week.
Comment Actions
I am working on a non-experimental series that get things more contained in repo/changectx to robustness. It also contains tests making sure we do not regress. I think we could so something really robust, efficient and transparent, but having this and that filtered changelog level, with lazyer filtering computation. However I think I dig the rabbit out enough for now. Eventually I think we could get O(1) filtering too removing the need for all this.
Sorry for the delay. I need a bit more (actual coding) time to clean this up and submit it.
Are you using the evolve extension. If so you should -already- not be loading obsmarkers (but there are other filtering associated cost). If you are using evolve and actually loading obsmarkers for this operation, we have a regression somewhere to fix first.