This is an archive of the discontinued Mercurial Phabricator instance.

Differential D477

revlog: add option to mmap revlog index
ClosedPublic

Authored by mbthomas on Aug 22 2017, 10:37 AM.

Details

Reviewers
indygreg
durin42
quark
Group Reviewers
Restricted Project
hg-reviewers
Commits
rHG3c9691728237: revlog: add option to mmap revlog index
Summary

Following on from Jun Wu's patch last October[1], we have found that using mmap
for the revlog index in repos with large revlogs gives a noticable performance
improvment (~110ms on each hg invocation), particularly for commands that don't
touch the index very much.

This changeset adds this as an option, activated by a new experimental config
option so that it can be enabled on a per-repo basis. The configuration option
specifies an index size threshold at which Mercurial will switch to using mmap
to access the index.

If the configuration option is not specified, the default remains to load the
full file, which seems to be the best option for smaller repos.

Some initial performance numbers for average of 5 invocations of hg log -l 5
for different cache states:

Repo:HGFB
Index size:2.3MBmuch bigger
read (warm):237ms432ms
mmap (warm):227ms321ms
(-3%)(-26%)
read (cold):397ms696ms
mmap (cold):410ms888ms
(+3%)(+28%)

[1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/088737.html

Test Plan

hg log --config experimental.mmapindex=true

Diff Detail

Repository
rHG Mercurial
Branch
mmap-revlog (bookmark) on default (branch)
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mbthomas created this revision.Aug 22 2017, 10:37 AM
simonfar added a subscriber: simonfar.Aug 23 2017, 9:05 AM
simonfar added inline comments.
mercurial/revlog.py
347

Is there any reason for mmapread to not immediately wrap the data in a util.buffer for you?

indygreg requested changes to this revision.Aug 23 2017, 11:31 AM

This seems reasonable as an experimental feature.

I worry about memory mapping all files without regards to size. That seems unnecessary.

Do you know what happens with the underlying file descriptor when it is memory mapped? If an fd stays open, that could lead to fd exhaustion. This can be mitigated by only using mmap for certain files (minimize size, changelog, etc). It's something I'd like an answer to before this is queued.

mercurial/localrepo.py
602–603

As you wrote in the commit message, the benefits of memory mapping likely kick in at some threshold and we'll want to make this behavior conditional on index size. It is tempting to implement that today to facilitate experimentation. If you can do that without introducing extra system calls for the default file open case, do it.

mercurial/revlog.py
332

We only support 2.7 now. So this version sniffing can be removed.

This revision now requires changes to proceed.Aug 23 2017, 11:31 AM
durin42 requested changes to this revision.Aug 23 2017, 11:58 AM
durin42 added a subscriber: durin42.

Also, please include at least one log invocation or similar in a test with the config knob set? And maybe some rough benchmark results in the commit message to justify the feature. :)

quark added a subscriber: quark.Aug 23 2017, 12:05 PM

I think it might make sense to only apply it to manifest and changelog. Filelogs are usually short and probably do not need it.

mbthomas added a comment.Aug 24 2017, 9:55 AM
In D477#7739, @indygreg wrote:

Do you know what happens with the underlying file descriptor when it is memory mapped? If an fd stays open, that could lead to fd exhaustion. This can be mitigated by only using mmap for certain files (minimize size, changelog, etc). It's something I'd like an answer to before this is queued.

The fd remains open. (Actually, the fd is dup'd and the original one is closed. The dup remains open.) For this reason I'm going to restrict it to changelog and manifest indexes only, as those are the ones that are known to grow big.

mercurial/revlog.py
347

I took it from Jun's original patch, but I think it's simply separation of responsibilities. util.mmapread's purpose is to wrap mmap(... ACCESS_READ) and handle the case where the file is empty, which normally fails.

mbthomas edited edge metadata.Aug 24 2017, 10:00 AM
mbthomas updated this revision to Diff 1245.

Switch to index size threshold for mmap, and only do it for changelog and manifest.
Rebase and add test.

mbthomas added a comment.Aug 24 2017, 10:02 AM

I've switched to a configurable threshold for mmap as suggested. This does require an extra stat call for the changelog and manifest indexes, however python already calls stat each time you open a file, so we should just hit the filesystem cache.

simonfar added inline comments.Aug 24 2017, 10:26 AM
tests/test-revlog-mmapindex.t
13–14

Two things:

  1. Can you add a ui.debug() or similar so that you can show that mmap() is in use here?
  2. Can you repeat the test without mmap() to show that mmap() and not-mmap() both still work?
mbthomas added inline comments.Aug 24 2017, 10:35 AM
tests/test-revlog-mmapindex.t
13–14

For (1) There's no ui object available in the revlog.revlog constructor. Is there a way to get one?
For (2), the rest of the test suite will be loading the revlog without mmap, so I didn't think it necessary to test it again here.

indygreg added inline comments.Aug 24 2017, 10:18 PM
tests/test-revlog-mmapindex.t
13–14

I'd do it with inline Python. Something like:

from mercurial import hg, ui as uimod
myui = uimod.ui.load()
repo = hg.repository(myui, path='.')

  1. Test repo.changelog here.

Also, excessive hg commands makes tests slow. Unfortunately, doing this in Python is probably a bit of work :/

quark added a comment.Aug 24 2017, 11:27 PM

I think revlog is decoupled from ui intentionally. I would avoid using ui.debug to test it, but just have a extension in .t that wraps the mmap method and print something to indicate mmap is being used.

mbthomas updated this revision to Diff 1294.Aug 25 2017, 11:58 AM

Make mmap test check for mmapping by making util.mmapread verbose

quark added inline comments.Aug 25 2017, 12:03 PM
tests/test-revlog-mmapindex.t
15

In this case verbosemmap.py is not very useful in other tests. The common pattern is to make it inside the .t file:

$ cat > verbosemmap.py <<EOF
> ...
> EOF
$ cat >> $HGRCPATH.py << EOF
> ...
> verbosemmap.py=$TESTTMP/verbosemmap.py
> EOF
mbthomas updated this revision to Diff 1297.Aug 25 2017, 2:46 PM

Move verbosemmap.py into the test

quark accepted this revision as: quark.Sep 11 2017, 6:34 PM

Generally look good to me.

mercurial/revlog.py
308

This variable is only used in __init__ so it is unnecessary to attach it to revlog object. In theory it could make a little difference if there are many revlog objects being created.

1526

I have checked this checkinlinesize code path and believe this patch isn't making things worse.

That said, I feel checkinlinesize could be troublesome in some cases. And maybe it's safer to move it to a post-close transaction hook.

mbthomas updated this revision to Diff 1786.Sep 13 2017, 1:27 PM

Rebase and remove mmapindexthreshold from revlog object

durin42 requested changes to this revision.Sep 15 2017, 11:25 AM

Please include benchmark numbers in your commit message.

This revision now requires changes to proceed.Sep 15 2017, 11:25 AM
mbthomas edited the summary of this revision. (Show Details)Sep 15 2017, 12:23 PM

I had put the numbers in the commit message, but that doesn't update the Phabricator summary, so I've done that manually.

It should be clear from the numbers that the improvement only really surfaces when the filesystem cache is warm, and there is a penalty for using this option when the filesystem cache is cold. Since the cache should be warm more often than not, the expectation is an overall improvement.

mbthomas requested review of this revision.Sep 15 2017, 12:28 PM
durham added a subscriber: durham.Sep 15 2017, 12:30 PM

Also, the improvements should become more pronounced if we fix some algorithms (like phases) that access really old commits.

quark added a comment.Sep 15 2017, 4:33 PM
In D477#12028, @mbthomas wrote:

I had put the numbers in the commit message, but that doesn't update the Phabricator summary, so I've done that manually.

I think you were using arc and can use hg phabsend to get the commit message updated. See https://www.mercurial-scm.org/wiki/Phabricator for instructions.

durham edited the summary of this revision. (Show Details)Sep 15 2017, 7:52 PM
quark added a comment.Sep 18 2017, 7:49 PM

237ms for HG seems there is something wrong.

durin42 requested changes to this revision.Sep 20 2017, 12:03 PM
durin42 added inline comments.
mercurial/revlog.py
348

This makes me very uncomfortable, because util.mmapread() returns the empty string if it doesn't like the file descriptor. I'd be less worried if util.mmapread() crashed, or if this was more cautious about the mmapread failing for some reason.

This revision now requires changes to proceed.Sep 20 2017, 12:03 PM
durin42 accepted this revision.Sep 21 2017, 11:36 AM

I'm landing this, but we should plan to figure out the right tradeoff values and get it on by default or discard the extra complexity at some point. That doesn't have to be before 4.4, but it'd be nice to have that sorted out by 4.5.

Closed by commit rHG3c9691728237: revlog: add option to mmap revlog index (authored by mbthomas). · Explain WhySep 21 2017, 2:05 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1294

mercurial/changelog.py

""" """
if trypending and opener.exists('00changelog.i.a'): if trypending and opener.exists('00changelog.i.a'):
indexfile = '00changelog.i.a' indexfile = '00changelog.i.a'
else: else:
indexfile = '00changelog.i' indexfile = '00changelog.i'
datafile = '00changelog.d' datafile = '00changelog.d'
revlog.revlog.__init__(self, opener, indexfile, datafile=datafile, revlog.revlog.__init__(self, opener, indexfile, datafile=datafile,
checkambig=True) checkambig=True, mmaplargeindex=True)
if self._initempty: if self._initempty:
# changelogs don't benefit from generaldelta # changelogs don't benefit from generaldelta
self.version &= ~revlog.FLAG_GENERALDELTA self.version &= ~revlog.FLAG_GENERALDELTA
self._generaldelta = False self._generaldelta = False
# Delta chains for changelogs tend to be very small because entries # Delta chains for changelogs tend to be very small because entries
# tend to be small and don't delta well with each. So disable delta # tend to be small and don't delta well with each. So disable delta

mercurial/localrepo.py

# experimental config: format.aggressivemergedeltas # experimental config: format.aggressivemergedeltas
aggressivemergedeltas = self.ui.configbool('format', aggressivemergedeltas = self.ui.configbool('format',
'aggressivemergedeltas') 'aggressivemergedeltas')
self.svfs.options['aggressivemergedeltas'] = aggressivemergedeltas self.svfs.options['aggressivemergedeltas'] = aggressivemergedeltas
self.svfs.options['lazydeltabase'] = not scmutil.gddeltaconfig(self.ui) self.svfs.options['lazydeltabase'] = not scmutil.gddeltaconfig(self.ui)
chainspan = self.ui.configbytes('experimental', 'maxdeltachainspan', -1) chainspan = self.ui.configbytes('experimental', 'maxdeltachainspan', -1)
if 0 <= chainspan: if 0 <= chainspan:
self.svfs.options['maxdeltachainspan'] = chainspan self.svfs.options['maxdeltachainspan'] = chainspan
mmapindexthreshold = self.ui.configbytes('experimental',
'mmapindexthreshold', None)
indygregUnsubmitted
Not Done

As you wrote in the commit message, the benefits of memory mapping likely kick in at some threshold and we'll want to make this behavior conditional on index size. It is tempting to implement that today to facilitate experimentation. If you can do that without introducing extra system calls for the default file open case, do it.

indygreg: As you wrote in the commit message, the benefits of memory mapping likely kick in at some…
if mmapindexthreshold is not None:
self.svfs.options['mmapindexthreshold'] = mmapindexthreshold
for r in self.requirements: for r in self.requirements:
if r.startswith('exp-compression-'): if r.startswith('exp-compression-'):
self.svfs.options['compengine'] = r[len('exp-compression-'):] self.svfs.options['compengine'] = r[len('exp-compression-'):]
# TODO move "revlogv2" to openerreqs once finalized. # TODO move "revlogv2" to openerreqs once finalized.
if REVLOGV2_REQUIREMENT in self.requirements: if REVLOGV2_REQUIREMENT in self.requirements:
self.svfs.options['revlogv2'] = True self.svfs.options['revlogv2'] = True

mercurial/manifest.py

# The dirlogcache is kept on the root manifest log # The dirlogcache is kept on the root manifest log
if dir: if dir:
self._dirlogcache = dirlogcache self._dirlogcache = dirlogcache
else: else:
self._dirlogcache = {'': self} self._dirlogcache = {'': self}
super(manifestrevlog, self).__init__(opener, indexfile, super(manifestrevlog, self).__init__(opener, indexfile,
# only root indexfile is cached # only root indexfile is cached
checkambig=not bool(dir)) checkambig=not bool(dir),
mmaplargeindex=True)
@property @property
def fulltextcache(self): def fulltextcache(self):
return self._fulltextcache return self._fulltextcache
def clearcaches(self): def clearcaches(self):
super(manifestrevlog, self).clearcaches() super(manifestrevlog, self).clearcaches()
self._fulltextcache.clear() self._fulltextcache.clear()

mercurial/revlog.py

Both pieces of the revlog are written to in an append-only Both pieces of the revlog are written to in an append-only
fashion, which means we never need to rewrite a file to insert or fashion, which means we never need to rewrite a file to insert or
remove data, and can use some simple techniques to avoid the need remove data, and can use some simple techniques to avoid the need
for locking while reading. for locking while reading.
If checkambig, indexfile is opened with checkambig=True at If checkambig, indexfile is opened with checkambig=True at
writing, to avoid file stat ambiguity. writing, to avoid file stat ambiguity.
If mmaplargeindex is True, and an mmapindexthreshold is set, the
index will be mmapped rather than read if it is larger than the
configured threshold.
""" """
def __init__(self, opener, indexfile, datafile=None, checkambig=False): def __init__(self, opener, indexfile, datafile=None, checkambig=False,
mmaplargeindex=False):
""" """
create a revlog object create a revlog object
opener is a function that abstracts the file opening operation opener is a function that abstracts the file opening operation
and can be used to implement COW semantics or the like. and can be used to implement COW semantics or the like.
""" """
self.indexfile = indexfile self.indexfile = indexfile
self.datafile = datafile or (indexfile[:-2] + ".d") self.datafile = datafile or (indexfile[:-2] + ".d")
self.index = [] self.index = []
# Mapping of partial identifiers to full nodes. # Mapping of partial identifiers to full nodes.
self._pcache = {} self._pcache = {}
# Mapping of revision integer to full node. # Mapping of revision integer to full node.
self._nodecache = {nullid: nullrev} self._nodecache = {nullid: nullrev}
self._nodepos = None self._nodepos = None
self._compengine = 'zlib' self._compengine = 'zlib'
self._maxdeltachainspan = -1 self._maxdeltachainspan = -1
self._mmapindexthreshold = None
quarkUnsubmitted
Not Done

This variable is only used in __init__ so it is unnecessary to attach it to revlog object. In theory it could make a little difference if there are many revlog objects being created.

quark: This variable is only used in `__init__` so it is unnecessary to attach it to revlog object. In…
v = REVLOG_DEFAULT_VERSION v = REVLOG_DEFAULT_VERSION
opts = getattr(opener, 'options', None) opts = getattr(opener, 'options', None)
if opts is not None: if opts is not None:
if 'revlogv2' in opts: if 'revlogv2' in opts:
# version 2 revlogs always use generaldelta. # version 2 revlogs always use generaldelta.
v = REVLOGV2 | FLAG_GENERALDELTA | FLAG_INLINE_DATA v = REVLOGV2 | FLAG_GENERALDELTA | FLAG_INLINE_DATA
elif 'revlogv1' in opts: elif 'revlogv1' in opts:
if 'generaldelta' in opts: if 'generaldelta' in opts:
v |= FLAG_GENERALDELTA v |= FLAG_GENERALDELTA
else: else:
v = 0 v = 0
if 'chunkcachesize' in opts: if 'chunkcachesize' in opts:
self._chunkcachesize = opts['chunkcachesize'] self._chunkcachesize = opts['chunkcachesize']
if 'maxchainlen' in opts: if 'maxchainlen' in opts:
self._maxchainlen = opts['maxchainlen'] self._maxchainlen = opts['maxchainlen']
if 'aggressivemergedeltas' in opts: if 'aggressivemergedeltas' in opts:
self._aggressivemergedeltas = opts['aggressivemergedeltas'] self._aggressivemergedeltas = opts['aggressivemergedeltas']
self._lazydeltabase = bool(opts.get('lazydeltabase', False)) self._lazydeltabase = bool(opts.get('lazydeltabase', False))
if 'compengine' in opts: if 'compengine' in opts:
self._compengine = opts['compengine'] self._compengine = opts['compengine']
if 'maxdeltachainspan' in opts: if 'maxdeltachainspan' in opts:
self._maxdeltachainspan = opts['maxdeltachainspan'] self._maxdeltachainspan = opts['maxdeltachainspan']
if mmaplargeindex and 'mmapindexthreshold' in opts:
indygregUnsubmitted
Not Done

We only support 2.7 now. So this version sniffing can be removed.

indygreg: We only support 2.7 now. So this version sniffing can be removed.
self._mmapindexthreshold = opts['mmapindexthreshold']
if self._chunkcachesize <= 0: if self._chunkcachesize <= 0:
raise RevlogError(_('revlog chunk cache size %r is not greater ' raise RevlogError(_('revlog chunk cache size %r is not greater '
'than 0') % self._chunkcachesize) 'than 0') % self._chunkcachesize)
elif self._chunkcachesize & (self._chunkcachesize - 1): elif self._chunkcachesize & (self._chunkcachesize - 1):
raise RevlogError(_('revlog chunk cache size %r is not a power ' raise RevlogError(_('revlog chunk cache size %r is not a power '
'of 2') % self._chunkcachesize) 'of 2') % self._chunkcachesize)
indexdata = '' indexdata = ''
self._initempty = True self._initempty = True
try: try:
f = self.opener(self.indexfile) f = self.opener(self.indexfile)
if (self._mmapindexthreshold is not None and
self.opener.fstat(f).st_size >= self._mmapindexthreshold):
simonfarUnsubmitted
Not Done

Is there any reason for mmapread to not immediately wrap the data in a util.buffer for you?

simonfar: Is there any reason for `mmapread` to not immediately wrap the data in a `util.buffer` for you?
mbthomasAuthorUnsubmitted
Not Done

I took it from Jun's original patch, but I think it's simply separation of responsibilities. util.mmapread's purpose is to wrap mmap(... ACCESS_READ) and handle the case where the file is empty, which normally fails.

mbthomas: I took it from Jun's original patch, but I think it's simply separation of responsibilities.
indexdata = util.buffer(util.mmapread(f))
durin42Unsubmitted
Not Done

This makes me very uncomfortable, because util.mmapread() returns the empty string if it doesn't like the file descriptor. I'd be less worried if util.mmapread() crashed, or if this was more cautious about the mmapread failing for some reason.

durin42: This makes me very uncomfortable, because util.mmapread() returns the empty string if it…
else:
indexdata = f.read() indexdata = f.read()
f.close() f.close()
if len(indexdata) > 0: if len(indexdata) > 0:
v = versionformat_unpack(indexdata[:4])[0] v = versionformat_unpack(indexdata[:4])[0]
self._initempty = False self._initempty = False
except IOError as inst: except IOError as inst:
if inst.errno != errno.ENOENT: if inst.errno != errno.ENOENT:
raise raise
df = self.opener(self.datafile, 'w') df = self.opener(self.datafile, 'w')
try: try:
for r in self: for r in self:
df.write(self._getsegmentforrevs(r, r)[1]) df.write(self._getsegmentforrevs(r, r)[1])
finally: finally:
df.close() df.close()
fp = self.opener(self.indexfile, 'w', atomictemp=True, fp = self.opener(self.indexfile, 'w', atomictemp=True,
quarkUnsubmitted
Not Done

I have checked this checkinlinesize code path and believe this patch isn't making things worse.

That said, I feel checkinlinesize could be troublesome in some cases. And maybe it's safer to move it to a post-close transaction hook.

quark: I have checked this `checkinlinesize` code path and believe this patch isn't making things…
checkambig=self._checkambig) checkambig=self._checkambig)
self.version &= ~FLAG_INLINE_DATA self.version &= ~FLAG_INLINE_DATA
self._inline = False self._inline = False
for i in self: for i in self:
e = self._io.packentry(self.index[i], self.node, self.version, i) e = self._io.packentry(self.index[i], self.node, self.version, i)
fp.write(e) fp.write(e)
# if we don't call close, the temp file will never replace the # if we don't call close, the temp file will never replace the

tests/test-revlog-mmapindex.t

  • This file was added.
setting up base repo
$ hg init a
$ cd a
$ touch a
$ hg add a
$ hg commit -qm base
$ for i in `seq 1 100` ; do
> echo $i > a
> hg commit -qm $i
> done
set up verbosemmap extension
$ cat << EOF >> $HGRCPATH
> [extensions]
simonfarUnsubmitted
Not Done

Two things:

  1. Can you add a ui.debug() or similar so that you can show that mmap() is in use here?
  2. Can you repeat the test without mmap() to show that mmap() and not-mmap() both still work?
simonfar: Two things: # Can you add a `ui.debug()` or similar so that you can show that `mmap()` is in…
mbthomasAuthorUnsubmitted
Not Done

For (1) There's no ui object available in the revlog.revlog constructor. Is there a way to get one?
For (2), the rest of the test suite will be loading the revlog without mmap, so I didn't think it necessary to test it again here.

mbthomas: For (1) There's no `ui` object available in the `revlog.revlog` constructor. Is there a way to…
indygregUnsubmitted
Not Done

I'd do it with inline Python. Something like:

from mercurial import hg, ui as uimod
myui = uimod.ui.load()
repo = hg.repository(myui, path='.')

  1. Test repo.changelog here.

Also, excessive hg commands makes tests slow. Unfortunately, doing this in Python is probably a bit of work :/

indygreg: I'd do it with inline Python. Something like: >>> from mercurial import hg, ui as uimod >>>…
> verbosemmap=$TESTDIR/verbosemmap.py
quarkUnsubmitted
Not Done

In this case verbosemmap.py is not very useful in other tests. The common pattern is to make it inside the .t file:

$ cat > verbosemmap.py <<EOF
> ...
> EOF
$ cat >> $HGRCPATH.py << EOF
> ...
> verbosemmap.py=$TESTTMP/verbosemmap.py
> EOF
quark: In this case `verbosemmap.py` is not very useful in other tests. The common pattern is to make…
> EOF
mmap index which is now more than 4k long
$ hg log -l 5 -T '{rev}\n' --config experimental.mmapindexthreshold=4k
mmapping $TESTTMP/a/.hg/store/00changelog.i (glob)
100
99
98
97
96
do not mmap index which is still less than 32k
$ hg log -l 5 -T '{rev}\n' --config experimental.mmapindexthreshold=32k
100
99
98
97
96
$ cd ..

tests/verbosemmap.py

  • This file was added.
# extension to make util.mmapread verbose
from __future__ import absolute_import
from mercurial import (
extensions,
util,
)
def mmapread(orig, fp):
print "mmapping %s" % fp.name
return orig(fp)
def extsetup(ui):
extensions.wrapfunction(util, 'mmapread', mmapread)