Patchwork D1460: workers: add config to enable/diable workers

login
register
mail settings
Submitter phabricator
Date Nov. 20, 2017, 6:37 p.m.
Message ID <differential-rev-PHID-DREV-wt3fqi7yfkdkrqis23pe-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25671/
State Superseded
Headers show

Comments

phabricator - Nov. 20, 2017, 6:37 p.m.
wlis created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This adds config to disable/enable workers with default being enabled.

TEST PLAN
  enabled profile without updaing .hg/hgrc (the default should be to use workers) and ran
  hg sprase --enable-profile <profile>.sparse
  Watched in the proces explorer that hg started 12 new threads for materializing files (this is my worker.numcpus) value
  
  Added
  
    [worker]
    enabled = False
  
  to the .hg/hgrc and re ran the command. This time hg didn't spawn any new threads for matreializing of files

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/help/config.txt
  mercurial/worker.py

CHANGE DETAILS




To: wlis, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 22, 2017, 8:28 a.m.
lothiraldan requested changes to this revision.
lothiraldan added a comment.
This revision now requires changes to proceed.


  Looks good.
  
  We are in the process of registering all the config options in the file named `configitems.py` (https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/configitems.py#l1132), could you update the file with the new option `worker.enabled`. By registering the option, we will be able to centralize default values (so you can even remove the default value in the `configbol` call) and do nice things like validating the user configuration or easily register aliases.

REPOSITORY
  rHG Mercurial

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

To: wlis, #hg-reviewers, lothiraldan
Cc: lothiraldan, mercurial-devel
phabricator - Dec. 17, 2017, 7:30 p.m.
indygreg added inline comments.

INLINE COMMENTS

> worker.py:85
>      '''
> -    if worthwhile(ui, costperarg, len(args)):
> +    enabled = ui.configbool('worker', 'enabled', True)
> +    if enabled and worthwhile(ui, costperarg, len(args)):

I updated this line to drop the default value argument, which is not needed when the config item defines a default.

The test harness passed with the original code. The developer warning is emitted at run time. So this tells me that we have 0 test coverage of this workers code :/

REPOSITORY
  rHG Mercurial

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

To: wlis, #hg-reviewers, lothiraldan, ikostia, durin42
Cc: indygreg, lothiraldan, mercurial-devel

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -82,7 +82,8 @@ 
     args - arguments to split into chunks, to pass to individual
     workers
     '''
-    if worthwhile(ui, costperarg, len(args)):
+    enabled = ui.configbool('worker', 'enabled', True)
+    if enabled and worthwhile(ui, costperarg, len(args)):
         return _platformworker(ui, func, staticargs, args)
     return func(*staticargs + (args,))
 
diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -2547,6 +2547,10 @@ 
 directory updates in parallel on Unix-like systems, which greatly
 helps performance.
 
+``enabled``
+    Whether to enable workers code to be used.
+    (default: true)
+
 ``numcpus``
     Number of CPUs to use for parallel operations. A zero or
     negative value is treated as ``use the default``.