-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Clean naming and disable stack pathhelper #3010
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,27 +10,30 @@ partial class Interop | |
partial class mincore | ||
{ | ||
/// <summary> | ||
/// WARNING: This overload does not implicitly handle long paths. | ||
/// WARNING: This method does not implicitly handle long paths. Use GetLongPathName. | ||
/// </summary> | ||
[DllImport(Libraries.CoreFile_L1, SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
internal unsafe static extern int GetLongPathNameW(char* path, char* longPathBuffer, int bufferLength); | ||
[DllImport(Libraries.CoreFile_L1, EntryPoint = "GetLongPathNameW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
internal unsafe static extern int GetLongPathNameUnsafe(char* path, char* longPathBuffer, int bufferLength); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
|
||
/// <summary> | ||
/// WARNING: This method does not implicitly handle long paths. Use GetLongPathName. | ||
/// </summary> | ||
[DllImport(Libraries.CoreFile_L1, EntryPoint = "GetLongPathNameW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
private static extern int GetLongPathNameWPrivate(string path, [Out]StringBuilder longPathBuffer, int bufferLength); | ||
private static extern int GetLongPathNamePrivate(string path, [Out]StringBuilder longPathBuffer, int bufferLength); | ||
|
||
internal static int GetLongPathNameW(string path, [Out]StringBuilder longPathBuffer, int bufferLength) | ||
internal static int GetLongPathName(string path, [Out]StringBuilder longPathBuffer, int bufferLength) | ||
{ | ||
bool wasExtended = PathInternal.IsExtended(path); | ||
if (!wasExtended) | ||
{ | ||
path = PathInternal.AddExtendedPathPrefixForLongPaths(path); | ||
path = PathInternal.EnsureExtendedPrefixOverMaxPath(path); | ||
} | ||
int result = GetLongPathNameWPrivate(path, longPathBuffer, longPathBuffer.Capacity); | ||
int result = GetLongPathNamePrivate(path, longPathBuffer, longPathBuffer.Capacity); | ||
|
||
if (!wasExtended) | ||
{ | ||
// We don't want to give back \\?\ if we possibly added it ourselves | ||
PathInternal.RemoveExtendedPathPrefix(longPathBuffer); | ||
PathInternal.RemoveExtendedPrefix(longPathBuffer); | ||
} | ||
return result; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,16 +176,7 @@ private unsafe static string NormalizePath(string path, bool fullCheck, int maxP | |
// since StringBuilder is used. | ||
// 2. IsolatedStorage, which supports paths longer than MaxPath (value given | ||
// by maxPathLength. | ||
PathHelper newBuffer = null; | ||
if (path.Length + 1 <= MaxPath) | ||
{ | ||
char* m_arrayPtr = stackalloc char[MaxPath]; | ||
newBuffer = new PathHelper(m_arrayPtr, MaxPath); | ||
} | ||
else | ||
{ | ||
newBuffer = new PathHelper(path.Length + MaxPath, maxPathLength); | ||
} | ||
PathHelper newBuffer = new PathHelper(path.Length + MaxPath, maxPathLength); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know the perf impact of this change? Should we still use stackalloc for smaller paths? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't because the stackalloc cannot grow currently. I've opened an issue on it and will fix so we can dynamically move up to the heap. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't there a known maximum amount by which it will need to grow, measured in just a few characters? If so, couldn't we just proactively make the stackalloc'd size that much larger? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically it shouldn't go beyond 520 characters (start with 260 relative and add to current directory). But there is no guarantee of that- I know of ways you can leak long paths into GetCurrentDirectory, and there might be others. There is also some hope that current directory will support > 260 in the future. I've got a lot of ideas about how to approach this issue- any pointers to perf experts would be appreciated. (One thought is a thread local string builder cache for path normalization.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If StringBuilderCache solves the need here, I'm fine with that. We'd want to make sure, though, that this particular usage didn't end up poisoning the cache, since IIRC StringBuilderCache only puts builders back into the cache if their capacity is below a threshold. |
||
|
||
uint numSpaces = 0; | ||
uint numDots = 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.
Why does this one have a different suffix? It can't be an overload of GetFullPathNamePrivate?
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.
Because it is internal and unsafe. It is used by the PathHelper class. Best I could come up with, but always open to suggestions.
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.
There are lots of methods that use unsafe code / take ptr arguments that don't have Unsafe in the name. More importantly, though, that's not the defining characteristic with regards to these various methods: the important thing we want to highlight is that it doesn't handle long paths / doesn't provide the wrapping behavior. How about using "Core" as the suffix, to highlight that it's the core functionality, and then the other things wrap it? Ideally we'd use it consistently instead of using "Private" in some places and other suffixes in others. If we can't do that, I'd prefer something like "Native" here, since it at least that highlights that this is the Win32 P/Invoke function.