[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mercurial-devel
Subject:    D588: win32: use fewer system calls for unlink()
From:       "indygreg (Gregory Szorc)" <phabricator () mercurial-scm ! org>
Date:       2017-08-31 22:32:43
Message-ID: differential-rev-PHID-DREV-7ml6wpgefgpmgndvxr4f-req () phab ! mercurial-scm ! org
[Download RAW message or body]

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:



To: indygreg, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[prev in list] [next in list] [prev in thread] [next in thread]