( )⚙ D808 config: use copy-on-write to improve copy performance

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D808

config: use copy-on-write to improve copy performance
ClosedPublic

Authored by quark on Sep 24 2017, 1:58 AM.

Details

Reviewers
mbthomas
durin42
Group Reviewers
hg-reviewers
Commits
rHGc41444a39de2: config: use copy-on-write to improve copy performance
Summary

Previously, chg's verify call could take 30+ms loading and checking new
config files. With one socket redirection, that adds up to around 70ms,
which is a lot for fast commands (ex. bookmark --hidden).

When investigating closer, A lot of time was spent on actually spent on ui
copying, which is mainly about config.config and dict copying.

This patch makes that 20x faster by adopting copy-on-write. The
copy-on-write is performed at config section level.

Before:

In [1]: %timeit ui.copy()
100 loops, best of 3: 2.32 ms per loop

After:

In [1]: %timeit ui.copy()
10000 loops, best of 3: 128 us per loop

2ms may look not that bad, but it adds up pretty quickly with multiple
calls. A typical chg run may call it 4 times, which is about 10ms.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 24 2017, 1:58 AM
quark edited the summary of this revision. (Show Details)Sep 24 2017, 2:10 AM
mbthomas requested changes to this revision.Sep 25 2017, 12:17 PM
mbthomas added a subscriber: mbthomas.

Requesting changes for the doc comment typo.

Otherwise looks correct. Up to you whether to follow up on the suggestions.

mercurial/util.py
592

Should be copy-on-write.

602

Using a boolean means both sides have to copy when they each write - the original dict becomes frozen and can't be re-used. If we use a count of outstanding copies instead, the last copy can just be used as-is when the count goes to 0.

I don't know how much modification of the original occurs after copy, so maybe this isn't important.

633

Any way we can check/enforce this? I'm guessing not without impacting perf.

This revision now requires changes to proceed.Sep 25 2017, 12:17 PM
quark edited the summary of this revision. (Show Details)Sep 25 2017, 7:47 PM
quark updated this revision to Diff 2072.
quark added inline comments.Sep 25 2017, 7:47 PM
mercurial/util.py
602

Doing refcount adds 10% overhead for ui.copy() benchmark. I guess it's fine. If we really care about it, we can use a native cow class.

633

I think we want performance here. If it becomes a concern, we can add extra checks if devel.something is set.

Side note: having a native class in inheritance hierarchy is important for CPython performance according to what I learned. I thought pure_obj.__contains__ = native_obj.__contains__ may have a reasonable perf, but the former is about 4x slower. So class pure(native) is preferred for perf (but probably not design pattern's point of view).

quark updated this revision to Diff 2073.Sep 25 2017, 7:51 PM
mbthomas accepted this revision.Sep 26 2017, 1:06 PM
durin42 accepted this revision as: durin42.Sep 27 2017, 7:48 PM
durin42 added a subscriber: durin42.

A couple of nitpicks. I'm not super in love with this, but it seems to be enough of a huge win we should do it anyway. Sigh.

mercurial/util.py
655

you can (and should) omit pass when you have a docstring

662

you can (and should) omit pass when you also have a docstring

quark added a comment.EditedSep 27 2017, 8:55 PM

I'm also sad about this. But we have a strong desire to make commands like status have native-like performance. So every 1ms counts. If we cannot deliver that performance, the team might eventually end up with a native-rewrite approach. I'll be even sadder if we have to do that.

I'm still optimistic though, as you can see the recent alias change removes 30ms overhead. The next big thing will be config caching - if statuses are not changed, do not re-parse the config. That's another 30-60ms for chg overhead.

quark updated this revision to Diff 2135.Sep 27 2017, 9:11 PM
durin42 accepted this revision.Sep 30 2017, 9:25 AM
This revision is now accepted and ready to land.Sep 30 2017, 9:25 AM
Closed by commit rHGc41444a39de2: config: use copy-on-write to improve copy performance (authored by quark). · Explain WhySep 30 2017, 9:39 AM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Sep 30 2017, 1:06 PM
yuja added inline comments.
mercurial/config.py
180

Perhaps we need preparewrite() here?

yuja added inline comments.Oct 1 2017, 8:32 AM
mercurial/config.py
180

This won't be problem as long as we parse/read file only once per config object,
so accepted. Please send a follow up.

Revision Contents
Changeset List

PathPackages
Mmercurial/config.py (24 lines)
Mmercurial/util.py (36 lines)

Diff 2058

mercurial/config.py

error, error,
pycompat, pycompat,
util, util,
) )
class config(object): class config(object):
def __init__(self, data=None, includepaths=None): def __init__(self, data=None, includepaths=None):
self._data = {} self._data = {}
self._source = {}
self._unset = [] self._unset = []
self._includepaths = includepaths or [] self._includepaths = includepaths or []
if data: if data:
for k in data._data: for k in data._data:
self._data[k] = data[k].copy() self._data[k] = data[k].copy()
self._source = data._source.copy() self._source = data._source.copy()
else:
self._source = util.cowdict()
def copy(self): def copy(self):
return config(self) return config(self)
def __contains__(self, section): def __contains__(self, section):
return section in self._data return section in self._data
def hasitem(self, section, item): def hasitem(self, section, item):
return item in self._data.get(section, {}) return item in self._data.get(section, {})
def __getitem__(self, section): def __getitem__(self, section):
return self._data.get(section, {}) return self._data.get(section, {})
def __iter__(self): def __iter__(self):
for d in self.sections(): for d in self.sections():
yield d yield d
def update(self, src): def update(self, src):
self._source = self._source.preparewrite()
for s, n in src._unset: for s, n in src._unset:
if s in self and n in self._data[s]: ds = self._data.get(s, None)
if ds is not None and n in ds:
self._data[s] = ds.preparewrite()
del self._data[s][n] del self._data[s][n]
del self._source[(s, n)] del self._source[(s, n)]
for s in src: for s in src:
if s not in self: ds = self._data.get(s, None)
self._data[s] = util.sortdict() if ds:
self._data[s] = ds.preparewrite()
else:
self._data[s] = util.cowsortdict()
self._data[s].update(src._data[s]) self._data[s].update(src._data[s])
self._source.update(src._source) self._source.update(src._source)
def get(self, section, item, default=None): def get(self, section, item, default=None):
return self._data.get(section, {}).get(item, default) return self._data.get(section, {}).get(item, default)
def backup(self, section, item): def backup(self, section, item):
"""return a tuple allowing restore to reinstall a previous value """return a tuple allowing restore to reinstall a previous value
return sorted(self._data.keys()) return sorted(self._data.keys())
def items(self, section): def items(self, section):
return list(self._data.get(section, {}).iteritems()) return list(self._data.get(section, {}).iteritems())
def set(self, section, item, value, source=""): def set(self, section, item, value, source=""):
if pycompat.ispy3: if pycompat.ispy3:
assert not isinstance(value, str), ( assert not isinstance(value, str), (
'config values may not be unicode strings on Python 3') 'config values may not be unicode strings on Python 3')
if section not in self: if section not in self:
self._data[section] = util.sortdict() self._data[section] = util.cowsortdict()
else:
self._data[section] = self._data[section].preparewrite()
self._data[section][item] = value self._data[section][item] = value
if source: if source:
self._source = self._source.preparewrite()
self._source[(section, item)] = source self._source[(section, item)] = source
def restore(self, data): def restore(self, data):
"""restore data returned by self.backup""" """restore data returned by self.backup"""
self._source = self._source.preparewrite()
if len(data) == 4: if len(data) == 4:
# restore old data # restore old data
section, item, value, source = data section, item, value, source = data
self._data[section] = self._data[section].preparewrite()
self._data[section][item] = value self._data[section][item] = value
self._source[(section, item)] = source self._source[(section, item)] = source
else: else:
# no data before, remove everything # no data before, remove everything
section, item = data section, item = data
if section in self._data: if section in self._data:
self._data[section].pop(item, None) self._data[section].pop(item, None)
self._source.pop((section, item), None) self._source.pop((section, item), None)
if emptyre.match(l): if emptyre.match(l):
continue continue
m = sectionre.match(l) m = sectionre.match(l)
if m: if m:
section = m.group(1) section = m.group(1)
if remap: if remap:
section = remap.get(section, section) section = remap.get(section, section)
if section not in self: if section not in self:
self._data[section] = util.sortdict() self._data[section] = util.cowsortdict()
continue continue
m = itemre.match(l) m = itemre.match(l)
if m: if m:
item = m.group(1) item = m.group(1)
cont = True cont = True
if sections and section not in sections: if sections and section not in sections:
continue continue
self.set(section, item, m.group(2), "%s:%d" % (src, line)) self.set(section, item, m.group(2), "%s:%d" % (src, line))
continue continue
m = unsetre.match(l) m = unsetre.match(l)
if m: if m:
name = m.group(1) name = m.group(1)
if sections and section not in sections: if sections and section not in sections:
continue continue
if self.get(section, name) is not None: if self.get(section, name) is not None:
del self._data[section][name] del self._data[section][name]
yujaUnsubmitted
Not Done

Perhaps we need preparewrite() here?

yuja: Perhaps we need preparewrite() here?
yujaUnsubmitted
Not Done

This won't be problem as long as we parse/read file only once per config object,
so accepted. Please send a follow up.

yuja: This won't be problem as long as we parse/read file only once per config object, so accepted.
self._unset.append((section, name)) self._unset.append((section, name))
continue continue
raise error.ParseError(l.rstrip(), ("%s:%s" % (src, line))) raise error.ParseError(l.rstrip(), ("%s:%s" % (src, line)))
def read(self, path, fp=None, sections=None, remap=None): def read(self, path, fp=None, sections=None, remap=None):
if not fp: if not fp:
fp = util.posixfile(path, 'rb') fp = util.posixfile(path, 'rb')

mercurial/util.py

else: else:
def f(*args): def f(*args):
if args not in cache: if args not in cache:
cache[args] = func(*args) cache[args] = func(*args)
return cache[args] return cache[args]
return f return f
class cow(object):
"""helper class to make copy-on-right easier"""
mbthomasUnsubmitted
Not Done

Should be copy-on-write.

mbthomas: Should be `copy-on-write`.
def preparewrite(self):
"""call this before writes, return a new object"""
if getattr(self, '_copied', False):
return self.__class__(self)
return self
def copy(self):
"""always do a cheap copy"""
self._copied = True
mbthomasUnsubmitted
Not Done

Using a boolean means both sides have to copy when they each write - the original dict becomes frozen and can't be re-used. If we use a count of outstanding copies instead, the last copy can just be used as-is when the count goes to 0.

I don't know how much modification of the original occurs after copy, so maybe this isn't important.

mbthomas: Using a boolean means both sides have to copy when they each write - the original dict becomes…
quarkAuthorUnsubmitted
Not Done

Doing refcount adds 10% overhead for ui.copy() benchmark. I guess it's fine. If we really care about it, we can use a native cow class.

quark: Doing refcount adds 10% overhead for `ui.copy()` benchmark. I guess it's fine. If we really…
return self
class sortdict(collections.OrderedDict): class sortdict(collections.OrderedDict):
'''a simple sorted dictionary '''a simple sorted dictionary
>>> d1 = sortdict([(b'a', 0), (b'b', 1)]) >>> d1 = sortdict([(b'a', 0), (b'b', 1)])
>>> d2 = d1.copy() >>> d2 = d1.copy()
>>> d2 >>> d2
sortdict([('a', 0), ('b', 1)]) sortdict([('a', 0), ('b', 1)])
>>> d2.update([(b'a', 2)]) >>> d2.update([(b'a', 2)])
if pycompat.ispypy: if pycompat.ispypy:
# __setitem__() isn't called as of PyPy 5.8.0 # __setitem__() isn't called as of PyPy 5.8.0
def update(self, src): def update(self, src):
if isinstance(src, dict): if isinstance(src, dict):
src = src.iteritems() src = src.iteritems()
for k, v in src: for k, v in src:
self[k] = v self[k] = v
class cowdict(cow, dict):
"""copy-on-write dict.
Be sure to call d = d.preparewrite() before writing to d.
mbthomasUnsubmitted
Not Done

Any way we can check/enforce this? I'm guessing not without impacting perf.

mbthomas: Any way we can check/enforce this? I'm guessing not without impacting perf.
quarkAuthorUnsubmitted
Not Done

I think we want performance here. If it becomes a concern, we can add extra checks if devel.something is set.

Side note: having a native class in inheritance hierarchy is important for CPython performance according to what I learned. I thought pure_obj.__contains__ = native_obj.__contains__ may have a reasonable perf, but the former is about 4x slower. So class pure(native) is preferred for perf (but probably not design pattern's point of view).

quark: I think we want performance here. If it becomes a concern, we can add extra checks if `devel.
>>> a = cowdict()
>>> a is a.preparewrite()
True
>>> b = a.copy()
>>> b is a
True
>>> a = a.preparewrite()
>>> b is a
False
>>> a is a.preparewrite()
True
"""
pass
class cowsortdict(cow, sortdict):
pass
class transactional(object): class transactional(object):
"""Base class for making a transactional type into a context manager.""" """Base class for making a transactional type into a context manager."""
__metaclass__ = abc.ABCMeta __metaclass__ = abc.ABCMeta
durin42Unsubmitted
Not Done

you can (and should) omit pass when you have a docstring

durin42: you can (and should) omit pass when you have a docstring
@abc.abstractmethod @abc.abstractmethod
def close(self): def close(self):
"""Successfully closes the transaction.""" """Successfully closes the transaction."""
@abc.abstractmethod @abc.abstractmethod
def release(self): def release(self):
"""Marks the end of the transaction. """Marks the end of the transaction.
durin42Unsubmitted
Not Done

you can (and should) omit pass when you also have a docstring

durin42: you can (and should) omit pass when you also have a docstring
If the transaction has not been closed, it will be aborted. If the transaction has not been closed, it will be aborted.
""" """
def __enter__(self): def __enter__(self):
return self return self
def __exit__(self, exc_type, exc_val, exc_tb): def __exit__(self, exc_type, exc_val, exc_tb):