Skip to content

mingw: improve performance of mingw_unlink() #264

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.

Sign up for GitHub

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

Merged
jeffhostetler merged 1 commit into from
Apr 20, 2020
Merged

mingw: improve performance of mingw_unlink() #264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,9 @@ int mingw_unlink(const char *pathname)
if (xutftowcs_long_path(wpathname, pathname) < 0)
return -1;

if (DeleteFileW(wpathname))
Copy link
Copy Markdown
Member

@kewillford kewillford Apr 20, 2020

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
Copy Markdown
Author

@jeffhostetler jeffhostetler Apr 20, 2020

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.)

return 0;

do {
/* read-only files cannot be removed */
_wchmod(wpathname, 0666);
Expand Down