This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3963

merge: mark file gets as not thread safe
ClosedPublic

Authored by indygreg on Jul 17 2018, 8:43 PM.

Details

Reviewers
lothiraldan
Group Reviewers
hg-reviewers
Commits
rHGbe4984261611: merge: mark file gets as not thread safe (issue5933)
Summary

In default installs, this has the effect of disabling the thread-based
worker on Windows when manifesting files in the working directory. My
measurements have shown that with revlog-based repositories, Mercurial
spends a lot of CPU time in revlog code resolving file data. This ends
up incurring a lot of context switching across threads and slows down
hg update operations when going from an empty working directory to
the tip of the repo.

On mozilla-unified (246,351 files) on an i7-6700K (4+4 CPUs):

before: 487s wall
after: 360s wall (equivalent to worker.enabled=false)
cpus=2: 379s wall

Even with only 2 threads, the thread pool is still slower.

The introduction of the thread-based worker (02b36e860e0b) states that
it resulted in a "~50%" speedup for hg sparse --enable-profile and
hg sparse --disable-profile. This disagrees with my measurement
above. I theorize a few reasons for this:

  1. Removal of files from the working directory is I/O - not CPU - bound and should benefit from a thread pool (unless I/O is insanely fast and the GIL release is near instantaneous). So tests like hg sparse --enable-profile may exercise deletion throughput and aren't good benchmarks for worker tasks that are CPU heavy.
  2. The patch was authored by someone at Facebook. The results were likely measured against a repository using remotefilelog. And I believe that revision retrieval during working directory updates with remotefilelog will often use a remote store, thus being I/O and not CPU bound. This probably resulted in an overstated performance gain.

Since there appears to be a need to enable the thread-based worker with
some stores, I've made the flagging of file gets as thread safe
configurable. I've made it experimental because I don't want to formalize
a boolean flag for this option and because this attribute is best
captured against the store implementation. But we don't have a proper
store API for this yet. I'd rather cross this bridge later.

It is possible there are revlog-based repositories that do benefit from
a thread-based worker. I didn't do very comprehensive testing. If there
are, we may want to devise a more proper algorithm for whether to use
the thread-based worker, including possibly config options to limit the
number of threads to use. But until I see evidence that justifies
complexity, simplicity wins.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Jul 17 2018, 8:43 PM
indygreg added a subscriber: wlis.Jul 17 2018, 8:45 PM

CC @wlis since this change will impact Facebook. I'd also appreciate validation of my assertions in the commit message about the behavior of remotefilelog and working directory updates being more I/O than CPU/GIL bound.

yuja added a subscriber: yuja.Jul 18 2018, 8:07 AM

+ cpuheavy = repo.ui.configbool('experimental', 'worker.wdir-get-cpu-heavy')

prog = worker.worker(repo.ui, cost, batchget, (repo, mctx, wctx),
  • actions[ACTION_GET])

+ actions[ACTION_GET],
+ cpuheavy=cpuheavy)

My two cents. It's better to add a flag to enable threading (e.g. threadsafe)
instead of cpuheavy, and make it off by default.

This is somewhat related to the issue5933. In short, merge functions aren't
thread safe if keywords extension is involved. I suspect the "fix" extension
would be also unsafe since it calls repo[rev].

https://bz.mercurial-scm.org/show_bug.cgi?id=5933

indygreg edited the summary of this revision. (Show Details)Jul 18 2018, 12:55 PM
indygreg retitled this revision from merge: mark file gets as CPU heavy to merge: mark file gets as not thread safe.
indygreg updated this revision to Diff 9624.
lothiraldan accepted this revision.Jul 19 2018, 3:06 AM
lothiraldan added a subscriber: lothiraldan.
lothiraldan added inline comments.
mercurial/merge.py
1641

Nit about a missing space before the =

Closed by commit rHGbe4984261611: merge: mark file gets as not thread safe (issue5933) (authored by indygreg). · Explain WhyJul 19 2018, 8:18 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/configitems.py (3 lines)
Mmercurial/merge.py (7 lines)

Diff 9631

mercurial/configitems.py

default=False, default=False,
) )
coreconfigitem('experimental', 'web.api.http-v2', coreconfigitem('experimental', 'web.api.http-v2',
default=False, default=False,
) )
coreconfigitem('experimental', 'web.api.debugreflect', coreconfigitem('experimental', 'web.api.debugreflect',
default=False, default=False,
) )
coreconfigitem('experimental', 'worker.wdir-get-thread-safe',
default=False,
)
coreconfigitem('experimental', 'xdiff', coreconfigitem('experimental', 'xdiff',
default=False, default=False,
) )
coreconfigitem('extensions', '.*', coreconfigitem('extensions', '.*',
default=None, default=None,
generic=True, generic=True,
) )
coreconfigitem('extdata', '.*', coreconfigitem('extdata', '.*',

mercurial/merge.py

f0, = args f0, = args
if wctx[f0].lexists(): if wctx[f0].lexists():
repo.ui.note(_("moving %s to %s\n") % (f0, f)) repo.ui.note(_("moving %s to %s\n") % (f0, f))
wctx[f].audit() wctx[f].audit()
wctx[f].write(wctx.filectx(f0).data(), wctx.filectx(f0).flags()) wctx[f].write(wctx.filectx(f0).data(), wctx.filectx(f0).flags())
wctx[f0].remove() wctx[f0].remove()
progress.increment(item=f) progress.increment(item=f)
# get in parallel # get in parallel.
threadsafe = repo.ui.configbool('experimental',
lothiraldanUnsubmitted
Not Done

Nit about a missing space before the =

lothiraldan: Nit about a missing space before the `=`
'worker.wdir-get-thread-safe')
prog = worker.worker(repo.ui, cost, batchget, (repo, mctx, wctx), prog = worker.worker(repo.ui, cost, batchget, (repo, mctx, wctx),
actions[ACTION_GET]) actions[ACTION_GET],
threadsafe=threadsafe)
for i, item in prog: for i, item in prog:
progress.increment(step=i, item=item) progress.increment(step=i, item=item)
updated = len(actions[ACTION_GET]) updated = len(actions[ACTION_GET])
if [a for a in actions[ACTION_GET] if a[0] == '.hgsubstate']: if [a for a in actions[ACTION_GET] if a[0] == '.hgsubstate']:
subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels) subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)
# forget (manifest only, just log it) (must come first) # forget (manifest only, just log it) (must come first)