D3963: merge: mark file gets as CPU heavy

2018-07-18 Thread yuja (Yuya Nishihara)
yuja added a comment.


  > +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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3963

To: indygreg, #hg-reviewers
Cc: yuja, wlis, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


Re: D3963: merge: mark file gets as CPU heavy

2018-07-18 Thread Yuya Nishihara
> +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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D3963: merge: mark file gets as CPU heavy

2018-07-17 Thread indygreg (Gregory Szorc)
indygreg added a subscriber: wlis.
indygreg added a comment.


  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3963

To: indygreg, #hg-reviewers
Cc: wlis, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D3963: merge: mark file gets as CPU heavy

2018-07-17 Thread indygreg (Gregory Szorc)
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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
  
  Even with the worker enabled with numcpus=2, I saw a slowdown (19s
  slower than no worker threads).
  
  The introduction of the thread-based worker 
(https://phab.mercurial-scm.org/rHG02b36e860e0b893928d5f565417d55b5dd6495fc) 
states that
  it resulted in a "~50%" speedup for `hg sparse --enable-profile` and
  `hg sparse --disable-profile`. 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` 
which don't realize new files are measuring 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 CPU heavy
  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3963

AFFECTED FILES
  mercurial/configitems.py
  mercurial/merge.py

CHANGE DETAILS

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1637,9 +1637,11 @@
 wctx[f0].remove()
 progress.increment(item=f)
 
-# get in parallel
+# get in parallel.
+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)
 for i, item in prog:
 progress.increment(step=i, item=item)
 updated = len(actions[ACTION_GET])
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -623,6 +623,9 @@
 coreconfigitem('experimental', 'web.api.debugreflect',
 default=False,
 )
+coreconfigitem('experimental', 'worker.wdir-get-cpu-heavy',
+default=True,
+)
 coreconfigitem('experimental', 'xdiff',
 default=False,
 )



To: indygreg, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel