(replying to this thread to see if the response shows up in Phabricator, Kevin just configured that)
> On Sep 1, 2017, at 02:16, Adrian Buehlmann <adr...@cadifra.com> wrote: > > Ugh. Can I reply to a phabricator notification by email? > > Adding gregory.sz...@gmail.com <mailto:gregory.sz...@gmail.com> manually, as > I'm not sure replaying to > those nasty phabricator emails is going to work... > > On 2017-09-01 00:32, indygreg (Gregory Szorc) wrote: >> indygreg created this revision. >> Herald added a subscriber: mercurial-devel. >> Herald added a reviewer: hg-reviewers. >> >> REVISION SUMMARY >> unlink() on Windows is complicated by the fact that Windows doesn't >> support removing an open file. Our unlink wrapper currently goes >> through a dance where it generates a random filename, renames the >> file, then attempts to unlink. >> >> This is a lot of extra work for what is likely the special case of >> attempting to unlink a file that is open or can't be simply unlinked. >> >> In this commit, we refactor the code to use fewer system calls in >> the common cases of 1) unlink just works 2) file doesn't exist. >> >> For the file doesn't exist case (before) >> >> 1. stat() to determine if path is directory >> 2. generate random filename >> 3. rename() >> >> after: >> >> 1. stat() to get path info >> >> For the file not open case (before) >> >> 1. stat() to determine if path is directory >> 2. generate random filename >> 3. rename() >> 4. unlink() >> >> after: >> >> 1. stat() >> 2. unlink() >> >> For the file open case (before) >> >> 1. stat() to determine if path is directory >> 2. generate random filename >> 3. rename() >> 4. unlink() >> >> after: >> >> 1. stat() >> 2. unlink() >> 3. generate random filename >> 4. rename() >> 5. unlink() >> >> There is also a scenario where the unlink fails due to the file being >> marked as read-only. In this case we also introduce an extra unlink() >> call. However, I reckon the common case is the file isn't marked as >> read-only and skipping the random generation and rename is worth it. >> >> I /think/ this change makes bulk file writing on Windows faster. This >> is because vfs.__call__ calls unlink() when a file is opened for >> writing. When writing a few hundred files files as part of a clone or >> working directory update, the extra system calls can matter. >> win32.unlink() did show up in performance profiles, which is what >> caused me to look at this code. But I/O performance is hard to measure >> and I'm not sure if the ~30s off of ~620s for a stream clone unbundle >> on the Firefox repo is indicative of real world performance. I do know >> the new code uses fewer system calls and shouldn't be slower. >> >> REPOSITORY >> rHG Mercurial >> >> REVISION DETAIL >> https://phab.mercurial-scm.org/D588 >> >> AFFECTED FILES >> mercurial/win32.py >> >> CHANGE DETAILS >> >> diff --git a/mercurial/win32.py b/mercurial/win32.py >> --- a/mercurial/win32.py >> +++ b/mercurial/win32.py >> @@ -12,6 +12,7 @@ >> import msvcrt >> import os >> import random >> +import stat >> import subprocess >> >> from . import ( >> @@ -522,11 +523,26 @@ >> def unlink(f): >> '''try to implement POSIX' unlink semantics on Windows''' >> >> - if os.path.isdir(f): >> - # use EPERM because it is POSIX prescribed value, even though >> - # unlink(2) on directories returns EISDIR on Linux >> - raise IOError(errno.EPERM, >> - "Unlinking directory not permitted: '%s'" % f) >> + # If the path doesn't exist, raise that exception. >> + # If it is a directory, emulate POSIX behavior. >> + try: >> + st = os.stat(f) >> + if stat.S_ISDIR(st.st_mode): >> + # use EPERM because it is POSIX prescribed value, even though >> + # unlink(2) on directories returns EISDIR on Linux >> + raise IOError(errno.EPERM, >> + "Unlinking directory not permitted: '%s'" % f) >> + except OSError as e: >> + if e.errno == errno.ENOENT: >> + raise >> + >> + # In the common case, a normal unlink will work. Try that first and fall >> + # back to more complexity if and only if we need to. >> + try: >> + os.unlink(f) >> + return >> + except (IOError, OSError) as e: >> + pass >> >> # POSIX allows to unlink and rename open files. Windows has serious >> # problems with doing that: > > Do you get an error at all, if a file, which is in open state, is unlinked? > > My fear is: You won't get an error, but instead the filename is blocked > by the file being held in place by the other process, until the other > process closes it. Which means: You already lost the game. > > Which would explain why we didn't do things like you propose here. > > See also > > https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows > <https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows> > > (specifically, heading 2) > > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org <mailto:Mercurial-devel@mercurial-scm.org> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel > <https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel>
_______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
