-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
#66754
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsAs described in #61954 (comment) this only provides minor improvements. In any case, an improvement of the memory allocation can be seen, since a lot has been spanified (see commit 6faa0e1). BenchmarksBefore
After
/cc @danmoseley
|
src/libraries/Common/src/System/IO/FileSystem.DirectoryCreation.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/IO/FileSystem.Attributes.Windows.cs
Outdated
Show resolved
Hide resolved
This reverts commit 8ce1a96.
Co-Authored-By: Günther Foidl <5755834+gfoidl@users.noreply.github.com>
I have updated the benchmarks. |
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FindFirstFileEx.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetFileAttributesEx.cs
Outdated
Show resolved
Hide resolved
github won't let me add this comment, but
|
I think there are problems on the part of GitHub. I just could not adjust the description of another PR. |
|
||
internal static bool EndsWithPeriodOrSpaceSlim(string path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this method can be turned into a single line and produce the same code:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHufuBLAHYYYUAdgA2TckgbAIESQEEAFMXIAGBgAdsGABYBKLrx6dqJi0wDs23XoDaO/QDoAMjAEBzfQzgNyALoMALzBDADkEQyEhLb6jnZuHt56vv5BoRHO4QDcxhYAvvkMxbyCwqISUjJyCgwAQqoacYalpm0WYHrYUAxgIS0JLu5ePn6BeeaWvMQ2/ZmRkTF9IWHh2ZPTRdQFQA===
(checked both 32 and 64 bit)
Given that, I suggest inlining path[path.Length - 1] == ' ' || path[path.Length - 1] == '.'
into EndsWithPeriodOrSpace and EnsureExtendedPrefixIfNeeded. This eliminates a method that isn't really useful, but also is a trap for anyone in future that calls it without checking the length. It's perfectly clear when inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, we have slightly changed the behavior here. Previously EndsWithPeriodOrSpace would return false for " " and now it will return true. Is that OK? I'm actually not sure.
If it is, then EndsWithPeriodOrSpace could be even less code: return path.Length > 0 && (path[path.Length - 1] == ' ' || path[path.Length - 1] == '.');
generates less code. But, I don't suggest changing that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have only two direct usages, which could break:
PathInternal.EnsureExtendedPrefixIfNeeded
[return: NotNullIfNotNull("path")]
internal static string? EnsureExtendedPrefixIfNeeded(string? path)
{
if (path != null && (path.Length >= MaxShortPath || EndsWithPeriodOrSpaceSlim(path)))
{
return EnsureExtendedPrefix(path);
}
else
{
return path;
}
}
and
FileSystemInfo.NormalizedPath
internal string NormalizedPath
=> PathInternal.EndsWithPeriodOrSpace(FullPath) ? PathInternal.EnsureExtendedPrefix(FullPath) : FullPath;
@tmds @jozkee Are you comfortable with this change (Tests are passing, but behavior might slightly change)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deeprobin I'd like to err on the side of caution here and avoid the subtle behavior change.
internal static int FillAttributeInfoSlim(string? path, ref Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA data, bool returnErrorOnNotFound) | ||
{ | ||
int errorCode = Interop.Errors.ERROR_SUCCESS; | ||
string? prefixedString = PathInternal.EnsureExtendedPrefixIfNeeded(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FindFirstFilePrefixed path here is the rare case, right? (<1%) Where the file is locked.
Given that, I don't think you're really saving anything by ensuring you only call EnsureExtendedPrefixIfNeeded once. I would reverse this and leave GetFileAttributesExPrivate private. That avoids creating a way for future code to accidentally call GetFileAttributesEx without prefixing. And it leaves this code a bit simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was marked resolved but I don't see the feedback addressed. Was it commented on somewhere else?
src/libraries/Common/src/System/IO/FileSystem.Attributes.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/IO/FileSystem.DirectoryCreation.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/IO/FileSystem.DirectoryCreation.Windows.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/IO/FileSystem.DirectoryCreation.Windows.cs
Outdated
Show resolved
Hide resolved
This pull request has been automatically marked |
Co-Authored-By: Dan Moseley <danmose@microsoft.com>
@deeprobin I got this branch caught back up with main and reran the checks; there are some tests failing related to the long paths and the affected code. On my local Windows 11 machine, when I set
|
I take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, but I'm unclear at this time what improvements the PR is concretely providing. Where are the cited allocation improvements coming from?
{ | ||
if (string.IsNullOrEmpty(path)) | ||
return false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the changes in this file necessary?
/// </summary> | ||
internal static bool EndsInDirectorySeparator(ReadOnlySpan<char> path) => | ||
path.Length > 0 && IsDirectorySeparator(path[path.Length - 1]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the changes in this file necessary?
@@ -986,7 +986,7 @@ private static string GetRelativePath(string relativeTo, string path, StringComp | |||
/// <summary> | |||
/// Returns true if the path ends in a directory separator. | |||
/// </summary> | |||
public static bool EndsInDirectorySeparator(ReadOnlySpan<char> path) => PathInternal.EndsInDirectorySeparator(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the changes in this file necessary?
|
||
if (!IsPathUnreachableError(errorCode)) | ||
using SafeFindHandle handle = Interop.Kernel32.FindFirstFile(prefixedString!, ref findData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this change?
(lastError == 0) && | ||
(data.dwFileAttributes != -1) && | ||
((data.dwFileAttributes & Interop.Kernel32.FileAttributes.FILE_ATTRIBUTE_DIRECTORY) != 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding these "Slim" functions seems to be an attempt to improve throughput, but the perf results shared don't really show a throughput improvement. Seems like these changes should be reverted?
internal static int FillAttributeInfoSlim(string? path, ref Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA data, bool returnErrorOnNotFound) | ||
{ | ||
int errorCode = Interop.Errors.ERROR_SUCCESS; | ||
string? prefixedString = PathInternal.EnsureExtendedPrefixIfNeeded(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was marked resolved but I don't see the feedback addressed. Was it commented on somewhere else?
I'm going to close this as it's unclear what the benefits are. Feel free to reopen if you address feedback and can show evidence of improvement making the change worthwhile. Thanks. |
Fixes #61954
As described in #61954 (comment) this only provides minor improvements.
Here it must be clearly decided which commits we take over and which are reverted again (Decision between micro improvement and code readability).
Personally, I also find it unnecessary that we use a
List
. However, I have not yet found a better way to get around this.Even if the benchmarks don't show a big improvement, the interop calls in case of an error have been reduced and so in case of a
throw
the performance is improved (even if this is not a hot path)In any case, an improvement of the memory allocation can be seen, since a lot has been spanified (see commit 6faa0e1).
Benchmarks
Before
After
/cc @danmoseley
/cc @adamsitnik
/cc @tmds