This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1457

workers: don't use backgroundfilecloser in threads
ClosedPublic

Authored by wlis on Nov 20 2017, 1:37 PM.

Details

Reviewers
indygreg
krbullock
Group Reviewers
hg-reviewers
Commits
rHG60f2a215faa7: workers: don't use backgroundfilecloser in threads
Summary

This disables background file closing when in not in main thread

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

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

wlis created this revision.Nov 20 2017, 1:37 PM
indygreg requested changes to this revision.Nov 20 2017, 10:55 PM
indygreg added a subscriber: indygreg.

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.

This revision now requires changes to proceed.Nov 20 2017, 10:55 PM
wlis added a comment.Nov 20 2017, 11:59 PM

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.

wlis edited the summary of this revision. (Show Details)Nov 30 2017, 7:01 PM
wlis retitled this revision from workers: create backgroundcloser per thread to workers: don't use backgroundfilecloser in threads.
wlis updated this revision to Diff 4022.
durin42 added a subscriber: durin42.Dec 5 2017, 5:23 PM
durin42 added inline comments.
mercurial/vfs.py
420–421

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

if (backgroundclose and

isinstance(...)):
krbullock requested changes to this revision.Dec 8 2017, 11:16 PM
krbullock added a subscriber: krbullock.

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

This revision now requires changes to proceed.Dec 8 2017, 11:16 PM
wlis updated this revision to Diff 4386.Dec 12 2017, 4:32 PM
wlis marked an inline comment as done.Dec 12 2017, 4:33 PM
Closed by commit rHG60f2a215faa7: workers: don't use backgroundfilecloser in threads (authored by wlis). · Explain WhyDec 15 2017, 12:13 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/vfs.py (11 lines)

Diff 4484

mercurial/vfs.py

@contextlib.contextmanager @contextlib.contextmanager
def backgroundclosing(self, ui, expectedcount=-1): def backgroundclosing(self, ui, expectedcount=-1):
"""Allow files to be closed asynchronously. """Allow files to be closed asynchronously.
When this context manager is active, ``backgroundclose`` can be passed When this context manager is active, ``backgroundclose`` can be passed
to ``__call__``/``open`` to result in the file possibly being closed to ``__call__``/``open`` to result in the file possibly being closed
asynchronously, on a background thread. asynchronously, on a background thread.
""" """
# This is an arbitrary restriction and could be changed if we ever # Sharing backgroundfilecloser between threads is complex and using
# have a use case. # multiple instances puts us at risk of running out of file descriptors
# only allow to use backgroundfilecloser when in main thread.
if not isinstance(threading.currentThread(), threading._MainThread):
yield
return
vfs = getattr(self, 'vfs', self) vfs = getattr(self, 'vfs', self)
if getattr(vfs, '_backgroundfilecloser', None): if getattr(vfs, '_backgroundfilecloser', None):
raise error.Abort( raise error.Abort(
_('can only have 1 active background file closer')) _('can only have 1 active background file closer'))
with backgroundfilecloser(ui, expectedcount=expectedcount) as bfc: with backgroundfilecloser(ui, expectedcount=expectedcount) as bfc:
try: try:
vfs._backgroundfilecloser = bfc vfs._backgroundfilecloser = bfc
self._fixfilemode(f) self._fixfilemode(f)
if checkambig: if checkambig:
if mode in ('r', 'rb'): if mode in ('r', 'rb'):
raise error.Abort(_('implementation error: mode %s is not' raise error.Abort(_('implementation error: mode %s is not'
' valid for checkambig=True') % mode) ' valid for checkambig=True') % mode)
fp = checkambigatclosing(fp) fp = checkambigatclosing(fp)
if backgroundclose: if (backgroundclose and
isinstance(threading.currentThread(), threading._MainThread)):
durin42Unsubmitted
Done

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

if (backgroundclose and

isinstance(...)):
durin42: nit: wrap lines using () instead of \, eg if (backgroundclose and isinstance(...)):
if not self._backgroundfilecloser: if not self._backgroundfilecloser:
raise error.Abort(_('backgroundclose can only be used when a ' raise error.Abort(_('backgroundclose can only be used when a '
'backgroundclosing context manager is active') 'backgroundclosing context manager is active')
) )
fp = delayclosedfile(fp, self._backgroundfilecloser) fp = delayclosedfile(fp, self._backgroundfilecloser)
return fp return fp