This is an archive of the discontinued Mercurial Phabricator instance.

Differential D474

perftweaks: store rev to node mapping for preloading
Needs RevisionPublic

Authored by mbthomas on Aug 22 2017, 8:46 AM.
Tags
None
Subscribers
quark

Details

Reviewers
durham
Group Reviewers
Restricted Project
Summary

Change perftweaks to store the node-to-rev mapping for interesting
revisions, rather than just the rev numbers.

Previously this was used to prepopulate the changelog's node-to-rev
lookup trie, however since we only stored the revision numbers this
still required looking at the revlog index contents in order to
read out the node hash.

With these changes, the trie can be prepopulated without touching
the index.

On its own, this gives only a modest performance increase, however
when combined with using mmap for the revlog index (coming in
another diff), it gives a noticable improvment on repos with very
large revlogs, as we no longer need to touch large portions of the
revlog index on every invocation.

When loading the cache data, we check that the rev numbers are
in range for the revlog, and drop any entries that are out of
range. The assumption is that a node hash won't be reassigned to
another rev index. The other way round ought to be ok, as the
old hash ought to be abandoned if it's no longer part of the
revlog. If there's any more checking needed, please suggest in
the review comments.

Test Plan

Re-run unit tests.
Generate cache data on a large repo and verify indexes can be
preloaded without touching the index contents.
Run with mmap change to verify perf improvement.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Branch
default
Lint
Lint OK
Unit
Unit Tests OK

Event Timeline

mbthomas created this revision.Aug 22 2017, 8:46 AM
durham requested changes to this revision.Aug 24 2017, 12:58 PM

I think we want to be pretty conservative about cache invalidation initially. If we get the map wrong, we could end up reading entirely incorrect data out of the changelog and associating it with the incorrect node. A few ideas:

  • We could initially treat the cache as risky, and instead of using the data we could just confirm that the data is always correct, by comparing the getnode(rev) result with our stored node->rev. If our cache doesn't match the truth, print a ui.develwarn so we see it. Once we have confidence, we can actually use the map result instead of calling getnode(rev).
  • We should invalidate the entire cache when repo.destroyed() is called. That is the function called at the end of a strip, to let extensions perform invalidation like this.
  • Maybe we could only use the cached mappings for commits older than tip - 10000. Any node that claims to be newer than tip-10000 we could do getnode(rev) and put the actual node value in the nodemap instead. Since the The top of the changelog is where the most churn happens and where the most risk will be, this should help prevent most bad cache issues and shouldn't add much IO slow down (10k revs is only 620KB of .i file)
hgext3rd/perftweaks.py
223

Is this better than revstr, node = line.split(':')?

Also, mercurial generally avoids using _ for unused variables, because _ is already the function name for the string internationalization function. I usually use x or some such

This revision now requires changes to proceed.Aug 24 2017, 12:58 PM
quark added a subscriber: quark.Nov 21 2017, 10:21 PM

This is no longer needed after D1472.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/perftweaks.py (62 lines)
Mtests/test-perftweaks.t (2 lines)

Diff 1155

hgext3rd/perftweaks.py

merge, merge,
phases, phases,
revlog, revlog,
scmutil, scmutil,
tags, tags,
util, util,
) )
from mercurial.extensions import wrapfunction from mercurial.extensions import wrapfunction
from mercurial.node import bin, nullid, nullrev from mercurial.node import bin, hex, nullid, nullrev
import errno import errno
import os import os
testedwith = 'ships-with-fb-hgext' testedwith = 'ships-with-fb-hgext'
cachedirname = 'noderevsmap'
def extsetup(ui): def extsetup(ui):
wrapfunction(tags, '_readtagcache', _readtagcache) wrapfunction(tags, '_readtagcache', _readtagcache)
wrapfunction(merge, '_checkcollision', _checkcollision) wrapfunction(merge, '_checkcollision', _checkcollision)
wrapfunction(branchmap.branchcache, 'update', _branchmapupdate) wrapfunction(branchmap.branchcache, 'update', _branchmapupdate)
if ui.configbool('perftweaks', 'preferdeltas'): if ui.configbool('perftweaks', 'preferdeltas'):
wrapfunction(revlog.revlog, '_isgooddelta', _isgooddelta) wrapfunction(revlog.revlog, '_isgooddelta', _isgooddelta)
wrapfunction(dispatch, 'runcommand', _trackdirstatesizes) wrapfunction(dispatch, 'runcommand', _trackdirstatesizes)
try: try:
remotenames = extensions.find('remotenames') remotenames = extensions.find('remotenames')
wrapfunction(remotenames, 'saveremotenames', _saveremotenames) wrapfunction(remotenames, 'saveremotenames', _saveremotenames)
except KeyError: except KeyError:
pass pass
def reposetup(ui, repo): def reposetup(ui, repo):
if repo.local() is not None: if repo.local() is not None:
_preloadrevs(repo) _preloadrevmap(repo)
def _readtagcache(orig, ui, repo): def _readtagcache(orig, ui, repo):
"""Disables reading tags if the repo is known to not contain any.""" """Disables reading tags if the repo is known to not contain any."""
if ui.configbool('perftweaks', 'disabletags'): if ui.configbool('perftweaks', 'disabletags'):
return (None, None, None, {}, False) return (None, None, None, {}, False)
return orig(ui, repo) return orig(ui, repo)
self.tiprev = tiprev self.tiprev = tiprev
self.filteredhash = scmutil.filteredhash(repo, self.tiprev) self.filteredhash = scmutil.filteredhash(repo, self.tiprev)
repo.ui.log('branchcache', 'perftweaks updated %s branch cache\n', repo.ui.log('branchcache', 'perftweaks updated %s branch cache\n',
repo.filtername) repo.filtername)
def _branchmapwrite(orig, self, repo): def _branchmapwrite(orig, self, repo):
result = orig(self, repo) result = orig(self, repo)
if repo.ui.configbool('perftweaks', 'cachenoderevs', True): if repo.ui.configbool('perftweaks', 'cachenoderevs', True):
revs = set() revs = {}
nodemap = repo.changelog.nodemap nodemap = repo.changelog.nodemap
for branch, heads in self.iteritems(): for branch, heads in self.iteritems():
revs.update(nodemap[n] for n in heads) revs.update((nodemap[n], n) for n in heads)
name = 'branchheads-%s' % repo.filtername name = 'branchheads-%s' % repo.filtername
_savepreloadrevs(repo, name, revs) _savenoderevmap(repo, name, revs)
return result return result
def _saveremotenames(orig, repo, remotepath, branches=None, bookmarks=None): def _saveremotenames(orig, repo, remotepath, branches=None, bookmarks=None):
result = orig(repo, remotepath, branches=branches, bookmarks=bookmarks) result = orig(repo, remotepath, branches=branches, bookmarks=bookmarks)
if repo.ui.configbool('perftweaks', 'cachenoderevs', True): if repo.ui.configbool('perftweaks', 'cachenoderevs', True):
revs = set() revs = {}
nodemap = repo.changelog.nodemap nodemap = repo.changelog.nodemap
if bookmarks: if bookmarks:
for b, n in bookmarks.iteritems(): for b, n in bookmarks.iteritems():
n = bin(n) n = bin(n)
# remotenames can pass bookmarks that don't exist in the # remotenames can pass bookmarks that don't exist in the
# changelog yet. It filters them internally, but we need to as # changelog yet. It filters them internally, but we need to as
# well. # well.
if n in nodemap: if n in nodemap:
revs.add(nodemap[n]) revs[nodemap[n]] = n
if branches: if branches:
for branch, nodes in branches.iteritems(): for branch, nodes in branches.iteritems():
for n in nodes: for n in nodes:
if n in nodemap: if n in nodemap:
revs.add(nodemap[n]) revs[nodemap[n]] = n
name = 'remotenames-%s' % remotepath name = 'remotenames-%s' % remotepath
_savepreloadrevs(repo, name, revs) _savenoderevmap(repo, name, revs)
return result return result
def _editphases(orig, self, repo, tr, *args): def _editphases(orig, self, repo, tr, *args):
result = orig(self, repo, tr, *args) result = orig(self, repo, tr, *args)
def _write(fp): def _write(fp):
revs = set() revs = {}
nodemap = repo.changelog.nodemap nodemap = repo.changelog.nodemap
for phase, roots in enumerate(self.phaseroots): for phase, roots in enumerate(self.phaseroots):
for n in roots: for n in roots:
if n in nodemap: if n in nodemap:
revs.add(nodemap[n]) revs[nodemap[n]] = n
_savepreloadrevs(repo, 'phaseroots', revs) _savenoderevmap(repo, 'phaseroots', revs)
# We don't actually use the transaction file generator. It's just a hook so # We don't actually use the transaction file generator. It's just a hook so
# we can write out at the same time as phases. # we can write out at the same time as phases.
if tr: if tr:
tr.addfilegenerator('noderev-phaseroot', ('phaseroots-fake',), _write) tr.addfilegenerator('noderev-phaseroot', ('phaseroots-fake',), _write)
else: else:
# fp is not used anyway # fp is not used anyway
_write(fp=None) _write(fp=None)
# full texts to be written over and over again. # full texts to be written over and over again.
if (l > textlen or compresseddeltalen > textlen * 2 or if (l > textlen or compresseddeltalen > textlen * 2 or
(self._maxchainlen and chainlen > self._maxchainlen)): (self._maxchainlen and chainlen > self._maxchainlen)):
return False return False
return True return True
def _cachefilename(name): def _cachefilename(name):
return 'noderevs/%s' % name return '/'.join((cachedirname, name))
def _preloadrevs(repo): def _preloadrevmap(repo):
# Preloading the node-rev map for likely to be used revs saves 100ms on # Preloading the node-rev map for likely to be used revs saves 100ms on
# every command. This is because normally to look up a node, hg has to scan # every command. This is because normally to look up a node, hg has to scan
# the changelog.i file backwards, potentially reading through hundreds of # the changelog.i file backwards, potentially reading through hundreds of
# thousands of entries and building a cache of them. Looking up a rev # thousands of entries and building a cache of them. Looking up a rev
# however is fast, because we know exactly what offset in the file to read. # however is fast, because we know exactly what offset in the file to read.
# Reading old commits is common, since the branchmap needs to to convert old # Reading old commits is common, since the branchmap needs to to convert old
# branch heads from node to rev. # branch heads from node to rev. To avoid touching the older parts of the
# index file, we just prepopulate the node to rev map.
if repo.ui.configbool('perftweaks', 'cachenoderevs', True): if repo.ui.configbool('perftweaks', 'cachenoderevs', True):
repo = repo.unfiltered() cachedir = repo.vfs.join('cache', cachedirname)
revs = set()
cachedir = repo.vfs.join('cache', 'noderevs')
try: try:
nodemap = repo.changelog.nodemap
maxrev = len(repo.changelog)
for cachefile in os.listdir(cachedir): for cachefile in os.listdir(cachedir):
filename = _cachefilename(cachefile) filename = _cachefilename(cachefile)
revs.update(int(r) for r in repo.cachevfs(filename)) for line in repo.cachevfs(filename):
revstr, _, node = line.partition(':')
durhamUnsubmitted
Not Done

Is this better than revstr, node = line.split(':')?

Also, mercurial generally avoids using _ for unused variables, because _ is already the function name for the string internationalization function. I usually use x or some such

durham: Is this better than `revstr, node = line.split(':')`? Also, mercurial generally avoids using…
getnode = repo.changelog.node rev = int(revstr)
nodemap = repo.changelog.nodemap if rev < maxrev:
for r in revs: nodemap[bin(node.strip())] = rev
try:
node = getnode(r)
nodemap[node] = r
except (IndexError, ValueError):
# Rev no longer exists or rev is out of range
pass
except EnvironmentError: except EnvironmentError:
# No permission to read? No big deal # No permission to read? No big deal
pass pass
def _savepreloadrevs(repo, name, revs): def _savenoderevmap(repo, name, revs):
if repo.ui.configbool('perftweaks', 'cachenoderevs', True): if repo.ui.configbool('perftweaks', 'cachenoderevs', True):
cachedir = repo.vfs.join('cache', 'noderevs') cachedir = repo.vfs.join('cache', cachedirname)
try: try:
repo.vfs.mkdir(cachedir) repo.vfs.mkdir(cachedir)
except OSError as ex: except OSError as ex:
# If we failed because the directory already exists, # If we failed because the directory already exists,
# continue. In all other cases (e.g., no permission to create the # continue. In all other cases (e.g., no permission to create the
# directory), just silently return without doing anything. # directory), just silently return without doing anything.
if ex.errno != errno.EEXIST: if ex.errno != errno.EEXIST:
return return
try: try:
filename = _cachefilename(name) filename = _cachefilename(name)
f = repo.cachevfs.open(filename, mode='w+', atomictemp=True) f = repo.cachevfs.open(filename, mode='w+', atomictemp=True)
f.write('\n'.join(str(r) for r in revs)) for r, n in revs.iteritems():
f.write('%s:%s\n' % (r, hex(n)))
f.close() f.close()
except EnvironmentError: except EnvironmentError:
# No permission to write? No big deal # No permission to write? No big deal
pass pass
def _trackdirstatesizes(runcommand, lui, repo, *args): def _trackdirstatesizes(runcommand, lui, repo, *args):
res = runcommand(lui, repo, *args) res = runcommand(lui, repo, *args)
if repo is not None and repo.local(): if repo is not None and repo.local():

tests/test-perftweaks.t

$ cd .. $ cd ..
$ mkdir permcheck $ mkdir permcheck
$ chmod g+ws permcheck $ chmod g+ws permcheck
$ cd permcheck $ cd permcheck
$ hg init repo $ hg init repo
$ cd repo $ cd repo
$ touch a $ touch a
$ hg commit -qAm a $ hg commit -qAm a
$ ls -la .hg/cache/noderevs/ $ ls -la .hg/cache/noderevsmap/
total * (glob) total * (glob)
drwxrw[sx]r-x.? [0-9]+ .* \. (re) drwxrw[sx]r-x.? [0-9]+ .* \. (re)
drwxrw[sx]r-x.? [0-9]+ .* \.\. (re) drwxrw[sx]r-x.? [0-9]+ .* \.\. (re)
-rw-rw-r--.? 1 .* branchheads-served (re) -rw-rw-r--.? 1 .* branchheads-served (re)
Test logging the dirsize and sparse profiles Test logging the dirsize and sparse profiles
Set up the sampling extension and set a log file, then do a repo status. Set up the sampling extension and set a log file, then do a repo status.