| Submitter | Gregory Szorc |
|---|---|
| Date | March 17, 2015, 6:08 a.m. |
| Message ID | <9e0d583a15c42f11d06b.1426572516@vm-ubuntu-main.gateway.sonic.net> |
| Download | mbox | patch |
| Permalink | /patch/8114/ |
| State | Rejected |
| Headers | show |
Comments
On Mon, 2015-03-16 at 23:08 -0700, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1426392860 25200 > # Sat Mar 14 21:14:20 2015 -0700 > # Branch stable > # Node ID 9e0d583a15c42f11d06b66684c9c9d85d9269d9f > # Parent b73a22d1d9bfe3a7f8633340ea75a0ab1526c21b > tags: create a supplemental cache of .hgtags filenodes (issue4550) The _only_ thing that makes this proposal appropriate for stable is that it fights a performance regression. Everything else about it screams not appropriate for stable. Especially the part where it grows without bounds. People in the real world often run the same hg -for years- and if they install this on a server, they could have a 1-to-1 ratio of cache entries to changelog entries. But this is also the first time I've seen the ALARMING numbers related to this bug, which is why I haven't been paying much attention. Specifically, they're not in your bug report ("dozens of seconds" with no baseline), which you self-triaged without cc:ing any guilty parties and thus slipped entirely past my radar. Have you actually identified the problematic change in 3.3? Can we back it out? I'm still tempted to re-release 3.2.4 as 3.3.x due to the number of untended regressions.
On Tue, Mar 17, 2015 at 2:36 PM, Matt Mackall <mpm@selenic.com> wrote: > On Mon, 2015-03-16 at 23:08 -0700, Gregory Szorc wrote: > > # HG changeset patch > > # User Gregory Szorc <gregory.szorc@gmail.com> > > # Date 1426392860 25200 > > # Sat Mar 14 21:14:20 2015 -0700 > > # Branch stable > > # Node ID 9e0d583a15c42f11d06b66684c9c9d85d9269d9f > > # Parent b73a22d1d9bfe3a7f8633340ea75a0ab1526c21b > > tags: create a supplemental cache of .hgtags filenodes (issue4550) > > The _only_ thing that makes this proposal appropriate for stable is that > it fights a performance regression. > > Everything else about it screams not appropriate for stable. Especially > the part where it grows without bounds. People in the real world often > run the same hg -for years- and if they install this on a server, they > could have a 1-to-1 ratio of cache entries to changelog entries. > > But this is also the first time I've seen the ALARMING numbers related > to this bug, which is why I haven't been paying much attention. > Specifically, they're not in your bug report ("dozens of seconds" with > no baseline), which you self-triaged without cc:ing any guilty parties > and thus slipped entirely past my radar. > > Have you actually identified the problematic change in 3.3? Can we back > it out? > > I'm still tempted to re-release 3.2.4 as 3.3.x due to the number of > untended regressions. I posted some numbers to the bug last night. I have not yet identified a regression range. In fact, I /think/ the evolve extension might have regressed (passing an unfiltered repo to something that reads tags). I think you've talked me out of trying to land this in stable. What do I need to change for this to land in default?
On Thu, 2015-03-19 at 20:50 -0700, Gregory Szorc wrote: > On Tue, Mar 17, 2015 at 2:36 PM, Matt Mackall <mpm@selenic.com> wrote: > > > On Mon, 2015-03-16 at 23:08 -0700, Gregory Szorc wrote: > > > # HG changeset patch > > > # User Gregory Szorc <gregory.szorc@gmail.com> > > > # Date 1426392860 25200 > > > # Sat Mar 14 21:14:20 2015 -0700 > > > # Branch stable > > > # Node ID 9e0d583a15c42f11d06b66684c9c9d85d9269d9f > > > # Parent b73a22d1d9bfe3a7f8633340ea75a0ab1526c21b > > > tags: create a supplemental cache of .hgtags filenodes (issue4550) > > > > The _only_ thing that makes this proposal appropriate for stable is that > > it fights a performance regression. > > > > Everything else about it screams not appropriate for stable. Especially > > the part where it grows without bounds. People in the real world often > > run the same hg -for years- and if they install this on a server, they > > could have a 1-to-1 ratio of cache entries to changelog entries. > > > > But this is also the first time I've seen the ALARMING numbers related > > to this bug, which is why I haven't been paying much attention. > > Specifically, they're not in your bug report ("dozens of seconds" with > > no baseline), which you self-triaged without cc:ing any guilty parties > > and thus slipped entirely past my radar. > > > > Have you actually identified the problematic change in 3.3? Can we back > > it out? > > > > I'm still tempted to re-release 3.2.4 as 3.3.x due to the number of > > untended regressions. > > > I posted some numbers to the bug last night. I have not yet identified a > regression range. In fact, I /think/ the evolve extension might have > regressed (passing an unfiltered repo to something that reads tags). > > I think you've talked me out of trying to land this in stable. > > What do I need to change for this to land in default? [resend] I'm still struggling to follow the story here, because the discussion is spread out over weeks and I've got dozens of things I'm trying to pay attention to. What I've gathered so far: there's a regression in tags computation caused by alternating between filtered and unfiltered views but we don't know when it was introduced or why. I'm not even sure if this is connected to the cset that got backed out anymore as there's no explicity link between it and the bug report and I have the attention span of a goldfish. Halp. Since I still don't know what introduced the problem, so I'm going to be super-reluctant to accept new and nontrivial code to fix it until someone says "this changeset caused the problem and we can't go back because <reasons> and we can't fix it because <reasons>". Can I please get you to focus on the first clause of that statement? Writing code before that point is way premature.
On Fri, Mar 20, 2015 at 12:08 PM, Matt Mackall <mpm@selenic.com> wrote: > On Thu, 2015-03-19 at 20:50 -0700, Gregory Szorc wrote: > > On Tue, Mar 17, 2015 at 2:36 PM, Matt Mackall <mpm@selenic.com> wrote: > > > > > On Mon, 2015-03-16 at 23:08 -0700, Gregory Szorc wrote: > > > > # HG changeset patch > > > > # User Gregory Szorc <gregory.szorc@gmail.com> > > > > # Date 1426392860 25200 > > > > # Sat Mar 14 21:14:20 2015 -0700 > > > > # Branch stable > > > > # Node ID 9e0d583a15c42f11d06b66684c9c9d85d9269d9f > > > > # Parent b73a22d1d9bfe3a7f8633340ea75a0ab1526c21b > > > > tags: create a supplemental cache of .hgtags filenodes (issue4550) > > > > > > The _only_ thing that makes this proposal appropriate for stable is > that > > > it fights a performance regression. > > > > > > Everything else about it screams not appropriate for stable. Especially > > > the part where it grows without bounds. People in the real world often > > > run the same hg -for years- and if they install this on a server, they > > > could have a 1-to-1 ratio of cache entries to changelog entries. > > > > > > But this is also the first time I've seen the ALARMING numbers related > > > to this bug, which is why I haven't been paying much attention. > > > Specifically, they're not in your bug report ("dozens of seconds" with > > > no baseline), which you self-triaged without cc:ing any guilty parties > > > and thus slipped entirely past my radar. > > > > > > Have you actually identified the problematic change in 3.3? Can we back > > > it out? > > > > > > I'm still tempted to re-release 3.2.4 as 3.3.x due to the number of > > > untended regressions. > > > > > > I posted some numbers to the bug last night. I have not yet identified a > > regression range. In fact, I /think/ the evolve extension might have > > regressed (passing an unfiltered repo to something that reads tags). > > > > I think you've talked me out of trying to land this in stable. > > > > What do I need to change for this to land in default? > > [resend] > > I'm still struggling to follow the story here, because the discussion is > spread out over weeks and I've got dozens of things I'm trying to pay > attention to. What I've gathered so far: there's a regression in tags > computation caused by alternating between filtered and unfiltered views > but we don't know when it was introduced or why. > > I'm not even sure if this is connected to the cset that got backed out > anymore as there's no explicity link between it and the bug report and I > have the attention span of a goldfish. Halp. > > Since I still don't know what introduced the problem, so I'm going to be > super-reluctant to accept new and nontrivial code to fix it until > someone says "this changeset caused the problem and we can't go back > because <reasons> and we can't fix it because <reasons>". Can I please > get you to focus on the first clause of that statement? Writing code > before that point is way premature. I've updated issue 4550 with my findings. AFAICT this is an outstanding issue. I accidentally flagged it as a regression because I converted my local repo to generaldelta around the time of 3.3, which made manifest resolution slower (longer delta chains), making my tags cache population times increase by a few minutes, causing me to suspect a regression in 3.3.
On Sat, 2015-03-21 at 11:30 -0700, Gregory Szorc wrote: > On Fri, Mar 20, 2015 at 12:08 PM, Matt Mackall <mpm@selenic.com> wrote: > > > On Thu, 2015-03-19 at 20:50 -0700, Gregory Szorc wrote: > > > On Tue, Mar 17, 2015 at 2:36 PM, Matt Mackall <mpm@selenic.com> wrote: > > > > > > > On Mon, 2015-03-16 at 23:08 -0700, Gregory Szorc wrote: > > > > > # HG changeset patch > > > > > # User Gregory Szorc <gregory.szorc@gmail.com> > > > > > # Date 1426392860 25200 > > > > > # Sat Mar 14 21:14:20 2015 -0700 > > > > > # Branch stable > > > > > # Node ID 9e0d583a15c42f11d06b66684c9c9d85d9269d9f > > > > > # Parent b73a22d1d9bfe3a7f8633340ea75a0ab1526c21b > > > > > tags: create a supplemental cache of .hgtags filenodes (issue4550) > > > > > > > > The _only_ thing that makes this proposal appropriate for stable is > > that > > > > it fights a performance regression. > > > > > > > > Everything else about it screams not appropriate for stable. Especially > > > > the part where it grows without bounds. People in the real world often > > > > run the same hg -for years- and if they install this on a server, they > > > > could have a 1-to-1 ratio of cache entries to changelog entries. > > > > > > > > But this is also the first time I've seen the ALARMING numbers related > > > > to this bug, which is why I haven't been paying much attention. > > > > Specifically, they're not in your bug report ("dozens of seconds" with > > > > no baseline), which you self-triaged without cc:ing any guilty parties > > > > and thus slipped entirely past my radar. > > > > > > > > Have you actually identified the problematic change in 3.3? Can we back > > > > it out? > > > > > > > > I'm still tempted to re-release 3.2.4 as 3.3.x due to the number of > > > > untended regressions. > > > > > > > > > I posted some numbers to the bug last night. I have not yet identified a > > > regression range. In fact, I /think/ the evolve extension might have > > > regressed (passing an unfiltered repo to something that reads tags). > > > > > > I think you've talked me out of trying to land this in stable. > > > > > > What do I need to change for this to land in default? > > > > [resend] > > > > I'm still struggling to follow the story here, because the discussion is > > spread out over weeks and I've got dozens of things I'm trying to pay > > attention to. What I've gathered so far: there's a regression in tags > > computation caused by alternating between filtered and unfiltered views > > but we don't know when it was introduced or why. > > > > I'm not even sure if this is connected to the cset that got backed out > > anymore as there's no explicity link between it and the bug report and I > > have the attention span of a goldfish. Halp. > > > > Since I still don't know what introduced the problem, so I'm going to be > > super-reluctant to accept new and nontrivial code to fix it until > > someone says "this changeset caused the problem and we can't go back > > because <reasons> and we can't fix it because <reasons>". Can I please > > get you to focus on the first clause of that statement? Writing code > > before that point is way premature. > > > I've updated issue 4550 with my findings. AFAICT this is an outstanding > issue. I accidentally flagged it as a regression because I converted my > local repo to generaldelta around the time of 3.3, which made manifest > resolution slower (longer delta chains), making my tags cache population > times increase by a few minutes, causing me to suspect a regression in 3.3. Ok, thanks. My suspicion is we're going to want to make something that explicitly follows the pattern of the new revbranchcache.. once that's stabilized.
Patch
diff --git a/mercurial/tags.py b/mercurial/tags.py --- a/mercurial/tags.py +++ b/mercurial/tags.py @@ -174,8 +174,40 @@ def _updatetags(filetags, tagtype, allta ahist.extend([n for n in bhist if n not in ahist]) alltags[name] = anode, ahist +# There are currently 2 files related to the tags cache: +# cache/tags and cache/hgtagsfnodes1. The former was the original +# cache and consists of two caches: a mapping of changeset to +# .hgtags filenodes and a caching of the result of tags resolution. +# The hgtags fnodes cache is simply a mapping of changeset nodes to +# .hgtags filenodes. +# +# The "tags" cache has known problems for users that have obsolescence +# enabled. +# +# The "tags" cache is concerned with the heads for the repository. For +# users of obsolescence, there are different sets of heads for the +# repository: the non-hidden heads and the hidden heads. If tags +# are computed against the normal/filtered repository, data about +# hidden heads won't be stored in the cache. If tags against the +# unfiltered/hidden heads are requested, the tags cache will need to +# lookup data for these hidden heads. This could lead to extra +# and potentially expensive computation, leading to significant +# performance issues. +# +# Currently, we work around the issue with the "tags" cache throwing +# away data on heads by supplementing that cache with a standalone +# cache of changeset node to .hgtags filenode. Looking up .hgtags +# filenodes can be expensive on large repositories, as it requires +# manifests to be resolved. With a dedicated filenode cache file that +# is immune to invalidation, we perform this lookup at most once +# and avoid the costliest aspect of "tags" cache resolution and +# invalidation. +# +# Ideally, there should be separate "tags" caches for each view +# of a repository, like how branchmap caches work. +# # The tag cache only stores info about heads, not the tag contents # from each head. I.e. it doesn't try to squeeze out the maximum # performance, but is simpler has a better chance of actually # working correctly. And this gives the biggest performance win: it @@ -276,26 +308,36 @@ def _readtagcache(ui, repo): newheads = [head for head in repoheads if head not in set(cacheheads)] + existingfnodes = _readhgtagsfnodescache(ui, repo) + manifestlookupcount = 0 + # Now we have to lookup the .hgtags filenode for every new head. # This is the most expensive part of finding tags, so performance # depends primarily on the size of newheads. Worst case: no cache # file, so newheads == repoheads. for head in reversed(newheads): + # Look in the supplemental hgtags fnodes cache first. + fnode = existingfnodes.get(head) + if fnode: + cachefnode[head] = fnode + continue + cctx = repo[head] try: fnode = cctx.filenode('.hgtags') cachefnode[head] = fnode + manifestlookupcount += 1 except error.LookupError: # no .hgtags file on this head pass duration = time.time() - starttime ui.log('tagscache', 'resolved %d tags cache entries from %d manifests in %0.4f ' 'seconds\n', - len(cachefnode), len(newheads), duration) + len(cachefnode), manifestlookupcount, duration) # Caller has to iterate over all heads, but can use the filenodes in # cachefnode to get to each .hgtags revision quickly. return (repoheads, cachefnode, None, True) @@ -345,4 +387,73 @@ def _writetagcache(ui, repo, heads, tagf try: cachefile.close() except (OSError, IOError): pass + + _updatehgtagsfnodescache(ui, repo, tagfnode) + +_fnodescachefile = 'cache/hgtagsfnodes1' + +def _readhgtagsfnodescache(ui, repo): + """Read the cache mapping changeset nodes to .hgtags filenodes. + + The cache consists of pairs of 20-byte nodes. + + No validation of the entries is performed other than a spot test + that the file doesn't contain extra data. Since nodes are derived + from content and are deterministic, mappings are constant. The + only thing that can change is that a changeset may be stripped. + Callers must perform their own checking to ensure "unknown" + entries don't leak out. + """ + data = repo.vfs.tryread(_fnodescachefile) + nodes = {} + l = len(data) + offset = 0 + while offset + 40 <= l: + node = data[offset:offset + 20] + fnode = data[offset + 20:offset + 40] + nodes[node] = fnode + offset += 40 + + ui.log('tagscache', + 'read %d entries from hgtags filenodes cache\n', + len(nodes)) + + # If we have data left over, something wasn't written properly. + # We remove the invalid cache and throw away any data that was + # read since we can't trust it. + if offset != l: + ui.warn(_('.hg/%s is corrupt; it will be rebuilt\n') % + _fnodescachefile) + repo.vfs.unlink(_fnodescachefile) + nodes = {} + + return nodes + +def _updatehgtagsfnodescache(ui, repo, fnodes): + """Update the cache file mapping changeset nodes to .hgtags fnodes. + + For now, all entries are preserved for all of time. In the future, we + should consider pruning this cache so its growth isn't unbounded. + """ + existing = _readhgtagsfnodescache(ui, repo) + + # Append new entries instead of re-writing existing content to reduce + # I/O. + missing = set(fnodes.keys()) - set(existing.keys()) + + entries = [] + # sorted() is to make tests sane. + for node in sorted(missing): + fnode = fnodes[node] + entries.append('%s%s' % (node, fnode)) + + data = ''.join(entries) + try: + repo.vfs.append(_fnodescachefile, data) + except (IOError, OSError): + pass + + ui.log('tagscache', + 'appended %d entries to hgtags filenodes cache\n', + len(missing)) diff --git a/tests/test-blackbox.t b/tests/test-blackbox.t --- a/tests/test-blackbox.t +++ b/tests/test-blackbox.t @@ -130,11 +130,13 @@ tags cache gets logged $ hg tag -m 'create test tag' test-tag $ hg tags tip 3:5b5562c08298 test-tag 2:d02f48003e62 - $ hg blackbox -l 3 + $ hg blackbox -l 5 1970/01/01 00:00:00 bob> resolved 1 tags cache entries from 1 manifests in ?.???? seconds (glob) 1970/01/01 00:00:00 bob> writing tags cache file with 2 heads and 1 tags + 1970/01/01 00:00:00 bob> read 0 entries from hgtags filenodes cache + 1970/01/01 00:00:00 bob> appended 1 entries to hgtags filenodes cache 1970/01/01 00:00:00 bob> tags exited 0 after ?.?? seconds (glob) extension and python hooks - use the eol extension for a pythonhook diff --git a/tests/test-tags.t b/tests/test-tags.t --- a/tests/test-tags.t +++ b/tests/test-tags.t @@ -2,8 +2,11 @@ Helper functions: $ cacheexists() { > [ -f .hg/cache/tags ] && echo "tag cache exists" || echo "no tag cache" > } + $ fnodescacheexists() { + > [ -f .hg/cache/hgtagsfnodes1 ] && echo "fnodes cache exists" || echo "no fnodes cache" + > } $ dumptags() { > rev=$1 > echo "rev $rev: .hgtags:" @@ -19,12 +22,16 @@ Setup: $ hg init t $ cd t $ cacheexists no tag cache + $ fnodescacheexists + no fnodes cache $ hg id 000000000000 tip $ cacheexists no tag cache + $ fnodescacheexists + no fnodes cache $ echo a > a $ hg add a $ hg commit -m "test" $ hg co @@ -32,14 +39,18 @@ Setup: $ hg identify acb14030fe0a tip $ cacheexists tag cache exists + $ fnodescacheexists + fnodes cache exists Try corrupting the cache $ printf 'a b' > .hg/cache/tags + $ printf 'extra' > .hg/cache/hgtagsfnodes1 $ hg identify .hg/cache/tags is corrupt, rebuilding it + .hg/cache/hgtagsfnodes1 is corrupt; it will be rebuilt acb14030fe0a tip $ cacheexists tag cache exists $ hg identify @@ -68,16 +79,16 @@ Create a tag behind hg's back: b9154636be93 tip Repeat with cold tag cache: - $ rm -f .hg/cache/tags + $ rm -f .hg/cache/tags .hg/cache/hgtagsfnodes1 $ hg identify b9154636be93 tip And again, but now unable to write tag cache: #if unix-permissions - $ rm -f .hg/cache/tags + $ rm -f .hg/cache/tags .hg/cache/hgtagsfnodes1 $ chmod 555 .hg $ hg identify b9154636be93 tip $ chmod 755 .hg @@ -327,9 +338,9 @@ Strip 2: destroy whole branch, no old he saved backup bundle to $TESTTMP/t3/.hg/strip-backup/*-backup.hg (glob) $ hg tags # partly stale tip 4:735c3ca72986 bar 0:bbd179dfa0a7 - $ rm -f .hg/cache/tags + $ rm -f .hg/cache/tags .hg/cache/hgtagsfnodes1 $ hg tags # cold cache tip 4:735c3ca72986 bar 0:bbd179dfa0a7