This is an archive of the discontinued Mercurial Phabricator instance.

workers: don't use backgroundfilecloser in threads
ClosedPublic

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

Details

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
Lint Skipped
Unit
Unit Tests Skipped

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
416

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
This revision was automatically updated to reflect the committed changes.