Patchwork Improve performance of win32.unlink() over CIFS/SMB

login
register
mail settings
Submitter Kaz Nishimura
Date Jan. 27, 2014, 6:45 a.m.
Message ID <CAObj8L4N7pz+WEbp0-SEgQpi9409Ah+_yfAaYud86S8_gw_Yog@mail.gmail.com>
Download mbox | patch
Permalink /patch/3408/
State Superseded
Headers show

Comments

Kaz Nishimura - Jan. 27, 2014, 6:45 a.m.
I wrote a patch that could improve performance of win32.unlink() over
CIFS/SMB but I want to confirm that it break nothing.  Could you test this
patch if you are using Mercurial on Windows, etiher over CIFS/SMB or on
local disks?  Because I am new to make a patch for Mercurial, please let me
know if I must change the patch further.

FYI, on my environment, I got the following timing results (both are from
the second runs):

Without the patch:

Z:\tmp\xllmnrd>e:hg update --time -r null
0 files updated, 0 files merged, 36 files removed, 0 files unresolved
time: real 9.378 secs (user 0.281+0.000 sys 1.123+0.000)

Z:\tmp\xllmnrd>e:hg update --time -r default
36 files updated, 0 files merged, 0 files removed, 0 files unresolved
time: real 5.432 secs (user 0.265+0.000 sys 0.749+0.000)

With the patch:

Z:\tmp\xllmnrd>e:hg update --time -r null
0 files updated, 0 files merged, 36 files removed, 0 files unresolved
time: real 4.427 secs (user 0.281+0.000 sys 0.749+0.000)

Z:\tmp\xllmnrd>e:hg update --time -r default
36 files updated, 0 files merged, 0 files removed, 0 files unresolved
time: real 4.418 secs (user 0.140+0.000 sys 0.593+0.000)



# HG changeset patch
# User Kaz Nishimura <kazssym@vx68k.org>
# Date 1381984037 -32400
#      Thu Oct 17 13:27:17 2013 +0900
# Branch stable
# Node ID d854b82381330cee14c01742243ad87af8677795
# Parent  dbf305f499613e6b833847b72f1eb5424f9331cc
win32: try to unlink files without renaming if possible

Unlinking a file with os.rename() and os.unlink() could be slow over CIFS
from
Windows clients due to its protocol overhead.  This patch changes
win32.unlink()
to try an exclusive open with the delete-on-close flag first and if a
sharing
violation is detected, to fall back to the original method.


     # POSIX allows to unlink and rename open files. Windows has serious
Pierre-Yves David - Jan. 27, 2014, 8:02 a.m.
On Mon, Jan 27, 2014 at 03:45:24PM +0900, Kaz Nishimura wrote:
> I wrote a patch that could improve performance of win32.unlink() over
> CIFS/SMB but I want to confirm that it break nothing.  Could you test this
> patch if you are using Mercurial on Windows, etiher over CIFS/SMB or on
> local disks?  Because I am new to make a patch for Mercurial, please let me
> know if I must change the patch further.

We are durrently in thei Mercurial 2.9 feature freeze. please resend this patch
after Febuary first.

More information here: http://mercurial.selenic.com/wiki/TimeBasedReleasePlan

Thanks for you contribution
Pierre-Yves David - April 14, 2014, 4 a.m.
Looks like this patches never got any attention. Can a windows user look 
at it?

On 01/27/2014 01:45 AM, Kaz Nishimura wrote:
> I wrote a patch that could improve performance of win32.unlink() over
> CIFS/SMB but I want to confirm that it break nothing.  Could you test
> this patch if you are using Mercurial on Windows, etiher over CIFS/SMB
> or on local disks?  Because I am new to make a patch for Mercurial,
> please let me know if I must change the patch further.
>
> FYI, on my environment, I got the following timing results (both are
> from the second runs):
>
> Without the patch:
>
> Z:\tmp\xllmnrd>e:hg update --time -r null
> 0 files updated, 0 files merged, 36 files removed, 0 files unresolved
> time: real 9.378 secs (user 0.281+0.000 sys 1.123+0.000)
>
> Z:\tmp\xllmnrd>e:hg update --time -r default
> 36 files updated, 0 files merged, 0 files removed, 0 files unresolved
> time: real 5.432 secs (user 0.265+0.000 sys 0.749+0.000)
>
> With the patch:
>
> Z:\tmp\xllmnrd>e:hg update --time -r null
> 0 files updated, 0 files merged, 36 files removed, 0 files unresolved
> time: real 4.427 secs (user 0.281+0.000 sys 0.749+0.000)
>
> Z:\tmp\xllmnrd>e:hg update --time -r default
> 36 files updated, 0 files merged, 0 files removed, 0 files unresolved
> time: real 4.418 secs (user 0.140+0.000 sys 0.593+0.000)
>
>
>
> # HG changeset patch
> # User Kaz Nishimura <kazssym@vx68k.org <mailto:kazssym@vx68k.org>>
> # Date 1381984037 -32400
> #      Thu Oct 17 13:27:17 2013 +0900
> # Branch stable
> # Node ID d854b82381330cee14c01742243ad87af8677795
> # Parent  dbf305f499613e6b833847b72f1eb5424f9331cc
> win32: try to unlink files without renaming if possible
>
> Unlinking a file with os.rename() and os.unlink() could be slow over
> CIFS from
> Windows clients due to its protocol overhead.  This patch changes
> win32.unlink()
> to try an exclusive open with the delete-on-close flag first and if a
> sharing
> violation is detected, to fall back to the original method.
>
> diff -r dbf305f49961 -r d854b8238133 mercurial/win32.py
> --- a/mercurial/win32.py
> +++ b/mercurial/win32.py
> @@ -25,4 +25,5 @@
>   # GetLastError
>   _ERROR_SUCCESS = 0
> +_ERROR_SHARING_VIOLATION = 32
>   _ERROR_INVALID_PARAMETER = 87
>   _ERROR_INSUFFICIENT_BUFFER = 122
> @@ -60,5 +61,7 @@
>   _OPEN_EXISTING = 3
>
> +_FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000
>   _FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
> +_FILE_FLAG_DELETE_ON_CLOSE = 0x04000000
>
>   # SetFileAttributes
> @@ -345,9 +348,16 @@
>       '''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 we could open f in this way, there must be no other open
> handles and
> +    # we could expect its name would be deleted immediately when we
> close it.
> +    # We also expect we will fail to open a directory.
> +    fh = _kernel32.CreateFileA(f, 0, 0, None, _OPEN_EXISTING,
> +        _FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_DELETE_ON_CLOSE, None)
> +    if fh != _INVALID_HANDLE_VALUE:
> +        _kernel32.CloseHandle(fh)
> +        return
> +    else:
> +        e = ctypes.WinError()
> +        if e.winerror != _ERROR_SHARING_VIOLATION:
> +            raise e
>
>       # POSIX allows to unlink and rename open files. Windows has serious
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Katsunori FUJIWARA - April 16, 2014, 12:43 p.m.
At Mon, 14 Apr 2014 00:00:06 -0400,
Pierre-Yves David wrote:
> 
> Looks like this patches never got any attention. Can a windows user look 
> at it?

Revised version should be already queued as 1a9ebc83a74c:

    http://selenic.com/repo/hg/rev/1a9ebc83a74c

even though this (old) version is still left in patchwork service.

    http://patchwork.serpentine.com/patch/3408/

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - April 16, 2014, 1:51 p.m.
On 04/16/2014 08:43 AM, FUJIWARA Katsunori wrote:
>
> At Mon, 14 Apr 2014 00:00:06 -0400,
> Pierre-Yves David wrote:
>>
>> Looks like this patches never got any attention. Can a windows user look
>> at it?
>
> Revised version should be already queued as 1a9ebc83a74c:
>
>      http://selenic.com/repo/hg/rev/1a9ebc83a74c

ah!, could not find it.

> even though this (old) version is still left in patchwork service.
>
>      http://patchwork.serpentine.com/patch/3408/

I've fixed the patchwok state, thanks.

Patch

diff -r dbf305f49961 -r d854b8238133 mercurial/win32.py
--- a/mercurial/win32.py
+++ b/mercurial/win32.py
@@ -25,4 +25,5 @@ 
 # GetLastError
 _ERROR_SUCCESS = 0
+_ERROR_SHARING_VIOLATION = 32
 _ERROR_INVALID_PARAMETER = 87
 _ERROR_INSUFFICIENT_BUFFER = 122
@@ -60,5 +61,7 @@ 
 _OPEN_EXISTING = 3

+_FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000
 _FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
+_FILE_FLAG_DELETE_ON_CLOSE = 0x04000000

 # SetFileAttributes
@@ -345,9 +348,16 @@ 
     '''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 we could open f in this way, there must be no other open handles
and
+    # we could expect its name would be deleted immediately when we close
it.
+    # We also expect we will fail to open a directory.
+    fh = _kernel32.CreateFileA(f, 0, 0, None, _OPEN_EXISTING,
+        _FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_DELETE_ON_CLOSE, None)
+    if fh != _INVALID_HANDLE_VALUE:
+        _kernel32.CloseHandle(fh)
+        return
+    else:
+        e = ctypes.WinError()
+        if e.winerror != _ERROR_SHARING_VIOLATION:
+            raise e