Clean naming and disable stack pathhelper #3010
Conversation
PathHelper doesn't handle growing past MAX_PATH when using the stackalloc version. #3009 tracks this.
| { | ||
| newBuffer = new PathHelper(path.Length + MaxPath, maxPathLength); | ||
| } | ||
| PathHelper newBuffer = new PathHelper(path.Length + MaxPath, maxPathLength); |
There was a problem hiding this comment.
Do we know the perf impact of this change? Should we still use stackalloc for smaller paths?
There was a problem hiding this comment.
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.
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.
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.
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.
|
Renames look reasonable to me. I just had the one question but you have filed an issue tracking that so you could use it for thinking about the perf impact as well. |
Clean naming and disable stack pathhelper
| [DllImport(Libraries.CoreFile_L1, SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
| internal unsafe static extern int GetFullPathNameW(char* path, int numBufferChars, char* buffer, IntPtr mustBeZero); | ||
| [DllImport(Libraries.CoreFile_L1, EntryPoint = "GetFullPathNameW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)] | ||
| internal unsafe static extern int GetFullPathNameUnsafe(char* path, int numBufferChars, char* buffer, IntPtr mustBeZero); |
There was a problem hiding this comment.
Why does this one have a different suffix? It can't be an overload of GetFullPathNamePrivate?
There was a problem hiding this comment.
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.
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.
Clean naming and disable stack pathhelper Commit migrated from dotnet/corefx@4ac67b6
PathHelper doesn't handle growing past MAX_PATH when using
the stackalloc version. #3009 tracks this.
@weshaggard, @stephentoub, @Priya91