This is an archive of the discontinued Mercurial Phabricator instance.

status: use unfiltered repo if we're getting status of working copy
AbandonedPublic

Authored by martinvonz on Oct 19 2019, 3:38 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Oct 19 2019, 3:38 AM

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.

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 ?
pulkit added a subscriber: pulkit.Oct 22 2019, 5:51 PM

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.

I have a couple of questions:

  • Can we get performance number with about without this patch ? (to gauge the relative gain)

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?

marmoute added inline comments.Oct 22 2019, 6:23 PM
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.

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

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

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.

I have a couple of questions:

  • Can we get performance number with about without this patch ? (to gauge the relative gain)

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?

@marmoute, could you explain what the issue is? And what function would I extract to help solve it?

The comment about maybe affecting extensions made me think of df463ca0adef. I never figured out what the issue was.

I have a couple of questions:

[…]

  • 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?

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.

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

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.

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 ?

The comment about maybe affecting extensions made me think of df463ca0adef. I never figured out what the issue was.

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 have a couple of questions:

[…]

  • 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?

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.

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

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.

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 ?

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)
martinvonz edited the summary of this revision. (Show Details)Nov 12 2019, 4:32 PM
martinvonz updated this revision to Diff 18044.
martinvonz marked an inline comment as done.Nov 12 2019, 4:35 PM

I've extracted a function for deciding whether to use an unfiltered repo, so hopefully this can get queued now.

durin42 accepted this revision.Nov 13 2019, 6:07 PM
This revision is now accepted and ready to land.Nov 13 2019, 6:07 PM

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.

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.

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.

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.

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

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.

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.

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.

That's fine. Thanks for the update. I'm glad you had not forgotten about it.

martinvonz abandoned this revision.Dec 10 2019, 2:41 AM

I'll abandon this in favor of D7492