Patchwork D1457: workers: create backgroundcloser per thread

login
register
mail settings
Submitter phabricator
Date Nov. 20, 2017, 6:37 p.m.
Message ID <differential-rev-PHID-DREV-74zwo4qmuusspyk4wbwe-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25668/
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 allows to create _backgroundfilecloser per thread.
  The threading.local() manages the distinct storage between threads and works for main thread as well (if no actuall threading is used)

TEST PLAN
  Ran pull, update, sparse commands and watched the closer threads created and destroyed in procexp.exe
  
  ran test on CentOS. No tests broken compared to the base

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/vfs.py

CHANGE DETAILS




To: wlis, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 21, 2017, 3:55 a.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  The main reason we have this restriction is because there is an upper limit to the number of open file descriptors a process can have. So if we have multiple instances and each is managing file closes for thousands of files, we could easily exhaust all available file descriptors. This would lead to random I/O failures (likely when trying to open a file), which would likely raise an uncaught exception and lead to an abort.
  
  So if we want to use multiple threads for workers on Windows, I think a better course of action is to reuse the singleton background file closer from all threads or not use a background file closer at all.

REPOSITORY
  rHG Mercurial

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

To: wlis, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Nov. 21, 2017, 4:59 a.m.
wlis added a comment.


  That sounds good. I actually started with a change to manage a single background closer between threads, but the locking code gets a bit complicated and seemed more risky. I didn't know the main reason for 1 background closer was the number of descriptors.
  I'll check what disabling backgroundcloser does to the performance.

REPOSITORY
  rHG Mercurial

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

To: wlis, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Dec. 5, 2017, 10:23 p.m.
durin42 added inline comments.

INLINE COMMENTS

> vfs.py:420
>  
> -        if backgroundclose:
> +        if backgroundclose and \
> +                isinstance(threading.currentThread(), threading._MainThread):

nit: wrap lines using () instead of \, eg

if (backgroundclose and

  isinstance(...)):

REPOSITORY
  rHG Mercurial

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

To: wlis, #hg-reviewers, indygreg
Cc: durin42, indygreg, mercurial-devel
phabricator - Dec. 9, 2017, 4:16 a.m.
krbullock requested changes to this revision.
krbullock added a comment.
This revision now requires changes to proceed.


  Looks okay to me except for Augie's nit. @indygreg ?

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -269,27 +269,27 @@ 
         for dirpath, dirs, files in os.walk(self.join(path), onerror=onerror):
             yield (dirpath[prefixlen:], dirs, files)
 
+    threaddata = threading.local()
+
     @contextlib.contextmanager
     def backgroundclosing(self, ui, expectedcount=-1):
         """Allow files to be closed asynchronously.
 
         When this context manager is active, ``backgroundclose`` can be passed
         to ``__call__``/``open`` to result in the file possibly being closed
         asynchronously, on a background thread.
         """
-        # This is an arbitrary restriction and could be changed if we ever
-        # have a use case.
         vfs = getattr(self, 'vfs', self)
-        if getattr(vfs, '_backgroundfilecloser', None):
+        if getattr(vfs.threaddata, '_backgroundfilecloser', None):
             raise error.Abort(
-                _('can only have 1 active background file closer'))
+                _('can only have 1 active background file closer per thread'))
 
         with backgroundfilecloser(ui, expectedcount=expectedcount) as bfc:
             try:
-                vfs._backgroundfilecloser = bfc
+                vfs.threaddata._backgroundfilecloser = bfc
                 yield bfc
             finally:
-                vfs._backgroundfilecloser = None
+                vfs.threaddata._backgroundfilecloser = None
 
 class vfs(abstractvfs):
     '''Operate files relative to a base directory
@@ -414,12 +414,12 @@ 
             fp = checkambigatclosing(fp)
 
         if backgroundclose:
-            if not self._backgroundfilecloser:
+            if not self.threaddata._backgroundfilecloser:
                 raise error.Abort(_('backgroundclose can only be used when a '
                                   'backgroundclosing context manager is active')
                                   )
 
-            fp = delayclosedfile(fp, self._backgroundfilecloser)
+            fp = delayclosedfile(fp, self.threaddata._backgroundfilecloser)
 
         return fp