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

Improve CreateDirectory on Windows #61954

Open
tmds opened this issue Nov 23, 2021 · 29 comments
Open

Improve CreateDirectory on Windows #61954

tmds opened this issue Nov 23, 2021 · 29 comments
Labels
area-System.IO help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone

Comments

@tmds
Copy link
Member

tmds commented Nov 23, 2021

The CreateDirectory implementation was improved on Unix by #58799 and #61777.

The first PR eliminates exists syscalls by deducting that information from the directory creation syscall. The second PR reduces allocations when creating parent directories by allocating the parent paths on the stack instead of creating strings.

Similar improvements can be made to the Windows implementation.

cc @adamsitnik @danmoseley

@tmds tmds added area-System.IO tenet-performance Performance related issue help wanted [up-for-grabs] Good issue for external contributors labels Nov 23, 2021
@ghost
Copy link

ghost commented Nov 23, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

The CreateDirectory implementation was improved on Unix by #58799 and #61777.

The first PR eliminates exists syscalls by deducting that information from the directory creation syscall. The second PR reduces allocations when creating parent directories by allocating the parent paths on the stack instead of creating strings.

Similar improvements can be made to the Windows implementation.

cc @adamsitnik @danmoseley

Author: tmds
Assignees: -
Labels:

area-System.IO, tenet-performance, up-for-grabs

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 23, 2021
@tmds tmds removed the untriaged New issue has not been triaged by the area owner label Nov 23, 2021
@adamsitnik adamsitnik added this to the Future milestone Nov 24, 2021
@danmoseley
Copy link
Member

@iSazonov you thumbsed up this, any interest in a PR? Seems relevant to Powershell. 😸

@iSazonov
Copy link
Contributor

@danmoseley PowerShell MSFT team agreed that PS community will start experimental work on new File System Provider in next milestone. We already have some API enhancement requests (important for PowerShell) in .Net repository and I hope we get them early so that we have a time for adoption. I ping-ed .Net in one issue but without answer - but I hope.
As for the issue, it is nice to have but it is not so critical as issues I mentioned.

@deeprobin
Copy link
Contributor

deeprobin commented Dec 15, 2021

Would like to have a look :)
If you want you can assign the issue to me.

I hope I can improve the performance in FileSystem.DirectoryCreation.Windows.cs

@danmoseley
Copy link
Member

@deeprobin it's yours.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2022
@deeprobin
Copy link
Contributor

Unfortunately, I could not achieve any relevant performance improvement here. I think the poor performance is mainly due to the WinAPI calls.

The rest is just minimal "validation overhead". We could perhaps make minimal improvements by using a ReadOnlySpan instead of a string in Path & PathInternal.

We could also analyze whether CreateDirectory is the best way to create a folder.
We should consider CreateDirectoryA and CreateDirectoryW.

I would look at it again this afternoon, however if anyone has any ideas please let me know.

@adamsitnik
Copy link
Member

We should consider CreateDirectoryA and CreateDirectoryW.

We are already using CreateDirectoryW:

[LibraryImport(Libraries.Kernel32, EntryPoint = "CreateDirectoryW", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)]
[return: MarshalAs(UnmanagedType.Bool)]
private static partial bool CreateDirectoryPrivate(
string path,
ref SECURITY_ATTRIBUTES lpSecurityAttributes);
internal static bool CreateDirectory(string path, ref SECURITY_ATTRIBUTES lpSecurityAttributes)
{
// We always want to add for CreateDirectory to get around the legacy 248 character limitation
path = PathInternal.EnsureExtendedPrefix(path);
return CreateDirectoryPrivate(path, ref lpSecurityAttributes);
}

and we can't use CreateDirectoryA as it has most likely same perf characteristics, but uses different encoding: https://stackoverflow.com/a/51462928/5852046

@adamsitnik
Copy link
Member

Unfortunately, I could not achieve any relevant performance improvement here. I think the poor performance is mainly due to the WinAPI calls.

Have you tried not checking if the directory exists but just trying to create it?

For other optimization (similar to #61777) you should see a memory drop. Which is always nice to have, even if it does not provide a major CPU time reduction.

@deeprobin
Copy link
Contributor

and we can't use CreateDirectoryA as it has most likely same perf characteristics, but uses different encoding: https://stackoverflow.com/a/51462928/5852046

So encoding would then probably only make a minimal performance difference.
Is CreateDirectoryA or W easier to encode?

Unfortunately, I could not achieve any relevant performance improvement here. I think the poor performance is mainly due to the WinAPI calls.

Have you tried not checking if the directory exists but just trying to create it?

For other optimization (similar to #61777) you should see a memory drop. Which is always nice to have, even if it does not provide a major CPU time reduction.

I'll take a look at it later today :D

@adamsitnik
Copy link
Member

We just have to use the *W methods

@danmoseley
Copy link
Member

danmoseley commented Mar 16, 2022

*A methods are just wrappers around the *W methods in all cases I'm aware of. They are guaranteed to be slower as well as being inappropriate

@deeprobin
Copy link
Contributor

@adamsitnik

internal static bool CreateDirectory(string path, ref SECURITY_ATTRIBUTES lpSecurityAttributes)
{
// We always want to add for CreateDirectory to get around the legacy 248 character limitation
path = PathInternal.EnsureExtendedPrefix(path);
return CreateDirectoryPrivate(path, ref lpSecurityAttributes);
}
}

In my opinion we should call PathInternal.EnsureExtendedPrefix(path) only if the length of the string is longer than 256.

Is there anything against this change?

@deeprobin
Copy link
Contributor

Is this check still up to date. I don't really understand it.

// We need this check to mask OS differences
// Handle CreateDirectory("X:\\") when X: doesn't exist. Similarly for n/w paths.
if ((count == 0) && !somepathexists)
{
string? root = Path.GetPathRoot(fullPath);
if (!DirectoryExists(root))
{
throw Win32Marshal.GetExceptionForWin32Error(Interop.Errors.ERROR_PATH_NOT_FOUND, root);
}
return;
}

@danmoseley
Copy link
Member

@deeprobin probably if you remove that code, then try to do CreateDirectory("x:\\") where x: doesn't exist, or you try to write to a non writeable path, the type of the exception you get will change. The code is apparently trying to ensure that you get the same exception on both Windows and Unix.

Another thing you could do is put in a Debug.Fail there somewhere temporarily, and run the tests. You will see which test hits it.

@deeprobin
Copy link
Contributor

I was only able to achieve a performance boost of 7us (& allocation reduce of 47B).
https://github.com/deeprobin/runtime/tree/issue-61954-rev

Before

Method Mean Error StdDev Median Min Max Allocated
CreateDirectory 180.1 us 6.94 us 7.43 us 177.6 us 169.2 us 195.9 us 497 B

After

Method Mean Error StdDev Median Min Max Allocated
CreateDirectory 173.7 us 3.70 us 3.64 us 173.9 us 166.8 us 178.7 us 450 B

Maybe one of you can find oppurnities to improve the performance.

ETL Profiling Artifact

dotTrace Timeline
Flamegraph

Unknown is mostly File I/O
Timeline Hotspot [Unknown]

@danmoseley
Copy link
Member

Thanks for the measurements. I'll let IO owners comment, but wanted to note that the standard deviation in those measurements is such that it's borderline whether it's meaningful improvement or not.

@danmoseley
Copy link
Member

Given the slim improvements, we would not want to make the code less maintainable. However I had a chance to take a look at your changes @deeprobin and several of them seem like worthwhile cleanup to remove unnecessary work.

I like how you kept things in their own commits so they could be reviewed individually.

Do you want to throw up a PR, and we can comment there? If there are changes you believe are dubious you could keep those out of the PR.

@iSazonov
Copy link
Contributor

I'd speculate a common use case is to create subdirectory in current folder (parent folders already exist).
So we could call CreateDirectoryW first and only fallback to creation of parents if it returns ERROR_PATH_NOT_FOUND error.
Also we can avoid splitting the path to stack and simply trim the path tail with Span.Slice.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 30, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2022
@deeprobin deeprobin removed their assignment Sep 5, 2022
@iSazonov
Copy link
Contributor

iSazonov commented Oct 28, 2022

By looking at this code more closely I think we can close this issue.

Obviously, creating a large number of new directories and even more so new nested directories (without having intermediate directories) are not common scenarios.

Common scenario is:

loop
    CreateNewDirectoryIfNotExist
    CreateOpenNewFile
    FillTheFile
loopend

Current Windows code addresses the scenario very well. This code first checks if a directory exists and only then tries to create it. The Unix code first tries to create a directory and then checks if it exists, i.e. it makes an extra call for this general scenario. I believe we don't want follow the pattern and to do extra pinvoke on Windows in the common scenario.
The Unix code pattern excludes one more pinvoke if creating intermediate directories nut again it is one time operation and not common scenario.

Using Unix code pattern and replacing string stack with int stack makes little sense too. Today all (5 or 6) used helper methods use string type for the path parameter. And in common scenario this would exclude one allocation if terminate slash presents.
It doesn't seem to be worth the effort until we want to convert all path API operations from strings to spans.

@tmds
Copy link
Member Author

tmds commented Oct 28, 2022

When I changed the Unix implementation to omit the exist checks, the rationale was that perf-sensitive code shouldn't try to create the same directory over and over again in a loop. That's what you describe here as the common scenario.

@iSazonov
Copy link
Contributor

iSazonov commented Oct 28, 2022

When I changed the Unix implementation to omit the exist checks, the rationale was that perf-sensitive code shouldn't try to create the same directory over and over again in a loop. That's what you describe here as the common scenario.

I don't understand what your comment is about. If you're talking about a high-performance scenario, there's only one way --- to create a directory and block it from being deleted - only then this code can stop worrying about recreating the directory. Optimizations made earlier in CreateDirectory (or which may have been made) have no effect on this scenario, since it excludes this code altogether.
We are left with only the scenario I described. In this scenario it is rather advantageous to check the existence of the directory before trying to create it.

@tmds
Copy link
Member Author

tmds commented Oct 28, 2022

I mean that if the new files created in a loop will always be written to the same directory, it's best to move directory creation out of the loop.

CreateNewDirectoryIfNotExist
loop
    CreateOpenNewFile
    FillTheFile
loopend

@iSazonov
Copy link
Contributor

I mean that if the new files created in a loop will always be written to the same directory, it's best to move directory creation out of the loop.

Only if we lock the directory. Otherwise another thread/process can remove it. So having CreateDirectory in the first place is more reliable and is common practice.

@tmds
Copy link
Member Author

tmds commented Oct 28, 2022

Most code doesn't account for directories that were just created go missing.
When that happens, an exception is thrown, and the operation aborts.

Moving the CreateNewDirectoryIfNotExist in the loop doesn't make the code thread-safe. It can even cause the directory to get re-created and mask the directory was deleted while the operation was on-going.

@iSazonov
Copy link
Contributor

You say the obvious things and again I don't understand how this relates to improving CreateDirectory on Windows requested in the issue.
I stated two things above:

  1. Usually we first check if a directory exists and only then create it. We can even find countless examples of code like if (!Directory.Exists) Directory.Create(). (I remember that we were fixing it in PowerShell code!)
    If, on the other hand, we put the pinvoke of directory creation first, this is only beneficial in the scenario where we need to create a lot of directories that are known to not exist. This is obviously a very rare scenario. So my preference is to have the existence check pinvoke in the first place.
  2. To implement int stack on Windows as it is done on Unix we need to duplicate at least 6 helper methods. At the same time this doesn't exclude all allocations at all - Path API is full of many different normalizations.
    So I don't see the point of int stack on Windows just for the sake of CreateDirectory.

Based on these two points, I conclude that we can close this issue.

A more promising issue should be that we need to analyze the entire Path API stack to see if it is possible to reduce the number of allocations.
Since this code is ported from the .Net Framework and saved as is for compatibility with the .Net Framework, we have hope that improvements are possible. Not an easy job, though.

@tmds
Copy link
Member Author

tmds commented Oct 29, 2022

You're trying to optimize for the case ensuring directories exist, while I'm trying to optimize for the case where new directories get created. Both are valid use-cases.

@NedAnd1
Copy link

NedAnd1 commented Aug 30, 2023

@danmoseley w/ the profiling provided by deeprobin I see room for improvement on the runtime side & in the flow of directory creation... I have some promising mockups, so I'd love to pick this up!

@deeprobin
Copy link
Contributor

Very good. If this is also okay on the part of the others, I am curious about your results!

A thought that still occurred to me would be perhaps to distinguish between the WinAPI calls, depending on whether the path is representable in ANSI (A call) or Unicode (W call).
But this would have to be checked before, if this brings a relevant performance boost.

@NedAnd1
Copy link

NedAnd1 commented Aug 31, 2023

A thought that still occurred to me would be perhaps to distinguish between the WinAPI calls, depending on whether the path is representable in ANSI (A call) or Unicode (W call). But this would have to be checked before, if this brings a relevant performance boost.

fyi .Net strings are already in the encoding used by CreateDirectoryW, plus the fact that CreateDirectoryA is a wrapper over CreateDirectoryW (converting the ANSI to Unicode under the hood) so it would only ever be slower (& the same is true at least for other file-system related win32 calls).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.IO help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
6 participants