[PATCH 3 of 3] streamclone: use backgroundfilecloser (issue4889)

Adrian Buehlmann adrian at cadifra.com
Tue Jan 19 03:23:53 CST 2016


On 2016-01-18 19:17, Gregory Szorc wrote:
> On Mon, Jan 18, 2016 at 12:51 AM, Adrian Buehlmann <adrian at cadifra.com
> <mailto:adrian at cadifra.com>> wrote:
> 
>     On 2016-01-18 03:44, Matt Mackall wrote:
>     > On Thu, 2016-01-14 at 13:45 -0800, Gregory Szorc wrote:
>     >> # HG changeset patch
>     >> # User Gregory Szorc <gregory.szorc at gmail.com
>     <mailto:gregory.szorc at gmail.com>>
>     >> # Date 1452807841 28800
>     >> #      Thu Jan 14 13:44:01 2016 -0800
>     >> # Node ID 3cc9fde87e3284c2653a25254b22f03339dbd075
>     >> # Parent  d73af0bac3890fd4b7b43d888ff61718a166cbdd
>     >> streamclone: use backgroundfilecloser (issue4889)
>     >
>     > I've gone ahead and queued these. I've left the 2048 default low to
>     make sure we
>     > maximize coverage during the freeze: please get people to test it!
> 
>     I won't test it myself (I stopped playing guinea pig quite a while ago),
>     but I think - as I already said - that a default of 2048 is way too low.
> 
> 
> My measurements on my i7-6700K (one of the fastest consumer grade CPUs
> available currently) show that file closes take 1-3ms. I arrived at 2048
> because I feel it is larger than most casual repositories but small
> enough to show benefits. At 1ms, 2048 closes is >2s. I don't think the
> 3.4x speedup in the commit message is reasonable because of startup and
> shutdown overhead on a smaller file set size. But even a 2x speedup will
> reduce operation time on 2049 files by >1s. And the impact only goes up
> from there. That's how I arrived at 2048.
>  
> 
>     This patch tries to "fix" a problem which does not exist for such small
>     numbers of files. The potential problems it may introduce are however
>     now inflicted on many users - due to the small default. On a platform,
>     which is already shaky enough (Windows).
> 
> 
> I understand your concern about Windows stability. Do you have any
> concrete concerns about potential problems this patch may introduce? We
> take care to limit the total number of open files. And the context
> managers ensure files are closed before locks are released. Exceptions
> in threads will get raised on the main thread. I'm not spotting an
> obvious weakness in this approach (other than potentially slowdown due
> to thread startup/shutdown overhead and more I/O contention - but I
> think it's OK to let the kernel sort these things out).

I don't have any known specifig bugs or specific questions myself at the
moment. If I would know a specifig bug, I would tell you.

But, for example, I think there was a remark by Yuya on Jan 12 ("Because
threads aren't joined, delayed closing can exceed the "with" scope. It
would cause race condition and too early lock releasing.") which I
haven't seen answered. Perhaps I missed a message though.

I wouldn't be surprised to see fun things, though. Windows notoriously
has problems with open files. I once noted a few on
UnlinkingFilesOnWindows in the wiki. People also keep putting
repositories on Windows shares, using virus scanners, etc. - despite
having been told not to do so - and then blame Mercurial for not being
"fit for production". See the mercurial mailing list for the latest rant
of this sort. Now add the chaotic nature of threads and the usual
programmer threading bugs to this picture. Yikes.

What I don't get is: Why don't you play guinea pig with mozilla-central
yourself first for one release cycle? This could be easily done by
increasing the default value.



More information about the Mercurial-devel mailing list