Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mingw: improve performance of mingw_unlink() #264

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

jeffhostetler
Copy link

Update mingw_unlink() to first try to delete the file with existing
permissions before trying to force it.

Windows throws an error when trying to delete a read-only file. The
mingw_unlink() compatibility wrapper always tries to _wchmod(666) the
file before calling _wunlink() to avoid that error. However, since
most files in the worktree are already writable, this is usually
wasted effort.

Update mingw_unlink() to just call DeleteFileW() directly and if that
succeeds return. If that fails, fall back into the existing code path
to update the permissions and use _wunlink() to get the existing
error code mapping.

Signed-off-by: Jeff Hostetler [email protected]

Update mingw_unlink() to first try to delete the file with existing
permissions before trying to force it.

Windows throws an error when trying to delete a read-only file.  The
mingw_unlink() compatibility wrapper always tries to _wchmod(666) the
file before calling _wunlink() to avoid that error.  However, since
most files in the worktree are already writable, this is usually
wasted effort.

Update mingw_unlink() to just call DeleteFileW() directly and if that
succeeds return.  If that fails, fall back into the existing code path
to update the permissions and use _wunlink() to get the existing
error code mapping.

Signed-off-by: Jeff Hostetler <[email protected]>
@@ -508,6 +508,9 @@ int mingw_unlink(const char *pathname)
if (xutftowcs_long_path(wpathname, pathname) < 0)
return -1;

if (DeleteFileW(wpathname))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding. Why is this not using _wunlink like below? What is this difference between DeleteFileW and _wunlink? Also should it be trying the _wrdir like below? Should lines 517-527 be pulled out into a method that can be called here and in the while loop?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a probabilistic effort. Most of the time your worktree is 0666 and DeleteFileW() will just succeed and be done. And most of the time you're trying to delete files rather than
directories. (That second point is speculation, but again I'm optimizing for the happy-path.)

_wunlink() is a routine in the MSFT CRT that just calls DeleteFileW() and then their custom (and private) GetLastError()-2-errno function if there's an error. So I'm just short-cutting
for the happy-path. But if that fails for any reason, I let the existing logic (both the
function calls and the loop) handle it so there's no semantic changes for the error
handling. (Some of this logic is a bit questionable, but that's outside of the scope
of what I wanted to do in this fix.)

@jeffhostetler
Copy link
Author

See also git-for-windows#2589

Copy link
Member

@kewillford kewillford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeffhostetler jeffhostetler merged commit e363258 into microsoft:vfs-2.26.1 Apr 20, 2020
@derrickstolee
Copy link

@jeffhostetler just btw I ran this on the office repo with git sparse-checkout set alternating between every Word* directory (expand) and the empty set (contract):

T1 T2 T3 Avg
expand old 47.7 40.4 38.9 42.33333
contract old 25.3 25.4 24.3 25
expand new 38.6 47.5 41.4 42.5 -0.00394
contract new 22.4 23.4 21.3 22.36667 0.105333

So this reduced the end-to-end time by 10% when deleting many files.

@jeffhostetler
Copy link
Author

very nice. thanks. i'm going run a set after i finish getting my desktop put back together.

i'd also like to run a set of numbers on branch switching between representative L1's or L2's
or whatever the typical developer in Office does.

@jeffhostetler
Copy link
Author

jeffhostetler commented Apr 21, 2020

I ran a series of timing tests on the Office repo on my Z440 desktop. This was a full enlistment at HEAD. I ran git sparse-checkout disable to populate the entire worktree and then git sparse-checkout init --cone to delete everything except the files at root. I repeated that sequence 3 times.

Each init needed to delete about 2.1M files.

git sparse-checkout init T1 T2 T3 Avg Savings
old 636.1s 638.3s 637.2s 637.2s
new 535.9s 544.8s 533.3s 538.0s 15.6%

In this example, run time was decreased by 15%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants