Change PathInternal.IsCaseSensitive to a constant #54340

lambdageek · 2021-06-17T16:05:11Z

See #54313 (comment) and dotnet/maui#822 (comment)

The code that initialized System.IO.PathInternal.s_isCaseSensitive did I/O in the temp folder to check for case sensitivity. This is a startup perf hit on Android where low-end devices may have very slow storage. It also returns results that may be misleading since different paths on Unix may have different filesystems mounted that have different case sensitivity (a common case on Android is ext4 for internal storage and exFAT for the SD card).

Instead, return a constant value based on the common platform behavior - case insensitive on Windows and Apple platforms, case sensitive elsewhere.

Also remove Path.IsCaseSensitive and Path.StringComparison - callers should use PathInternal properties directly.

Fixes #54339

dotnet-issue-labeler · 2021-06-17T16:05:16Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2021-06-17T16:12:37Z

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

Issue Details

See #54313 (comment) and dotnet/maui#822 (comment)

The code that initializes System.IO.PathInternal.s_isCaseSensitive checks does I/O in the temp folder to check for case sensitivity. This is a startup perf hit on Android where low-end devices may have very slow storage. It also returns results that may be misleading since different paths on Unix may have different filesystems mounted that have different case sensitivity (a common case on Android is ext4 for internal storage and exFAT for the SD card).

We should simply return true here and assume case sensitivity.

Fixes #54339

Author:	lambdageek
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

jkotas · 2021-06-17T19:25:32Z

Would this make sense to hardcode this for all mobile targets ? (e.g. have two files - *.CaseSensitive.cs and *.CaseInsensitive.cs and include them as appropriate).

marek-safar

I think it would make sense to hardcode this for known behaviour (e.g. iOS like or browser-wasm).

I'm not sure why there is similar logic in Path class as well

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs

Line 961 in 01b7e73

internal static StringComparison StringComparison =>

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.Android.cs

jkotas · 2021-06-18T12:12:57Z

I'm not sure why there is similar logic in Path class as well

The behavior is actually hardcoded in Path class for all platforms:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs

Line 137 in 01b7e73

#if TARGET_OSX || TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS

The reason for this duplication is that there is no public Path.IsCaseSensitive property, but this property is needed by outside of CoreLib. PathInternal is included by outside of CoreLib to get access to IsCaseSensitive and several other internal properties.

We should make the logic in PathInternal to be hardcoded in the same way as the logic in Path. It helps with both performance and consistency. The check in PathInternal will need to be dynamic (using OperatingSystem.Is*).

lambdageek · 2021-06-18T13:18:26Z

I'm following along with most of the discussion, but I don't understand this part @jkotas:

The check in PathInternal will need to be dynamic (using OperatingSystem.Is*).

Aren't we talking about doing the same thing in PathInternal as in Path: just hardcode based on the common behavior of the underlying OS? Why would it need to be a dynamic check if we know which RID we're targeting?

marek-safar · 2021-06-18T13:47:34Z

@lambdageek because the file is shared as a compilation file you cannot assume that the assembly where the file is included is multi-targeting (has RID specific builds). OperatingSystem properties are static constants so I don't think there will be any perf hit.

lambdageek · 2021-06-18T17:36:36Z

Ok, updated to return constants for some known OperatingSystem.IsXYZ values. I left the probing in place as the last case (so non-Android Linux, Windows, FreeBSD and "none of the above" will still do the I/O probing)

…orms In particular on Android probing using I/O is slow and contributes to slow app startup. Fixes dotnet#54339

jkotas · 2021-06-18T18:56:43Z

I do not think we should be doing any probing. The IsCaseInsensitive property on Path is not doing any probing, and it better be in sync with the property on PathInternal. The probing on PathInternal can only ever lead to inconsistent behavior.

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs

jkotas · 2021-06-18T19:04:25Z

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs

+#if NET6_0_OR_GREATER
+            // Return a constant for mobile platforms without resorting to I/O
+            if (OperatingSystem.IsMacOS() || OperatingSystem.IsMacCatalyst() || OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsWatchOS())
+                return true;


I think this is backwards - it should return false for MacOS.

jkotas · 2021-06-18T19:06:23Z

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs

+        {
+#if NET6_0_OR_GREATER
+            // Return a constant for mobile platforms without resorting to I/O
+            if (OperatingSystem.IsMacOS() || OperatingSystem.IsMacCatalyst() || OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsWatchOS())


The ifdef in Path.Unix.cs does not have WatchOS. Is it a bug?

adamsitnik · 2021-06-21T08:02:26Z

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs

        /// <remarks>
        /// Ideally we'd use something like pathconf with _PC_CASE_SENSITIVE, but that is non-portable,
        /// not supported on Windows or Linux, etc. For now, this function creates a tmp file with capital letters
        /// and then tests for its existence with lower-case letters.  This could return invalid results in corner
        /// cases where, for example, different file systems are mounted with differing sensitivities.
        /// </remarks>
-        private static bool GetIsCaseSensitive()
+        private static bool GetIsCaseSensitiveByProbing()


I think that it would be great to use the old implementation as a test of the new one.

You could introduce a new test in System.IO.FileSystem.Tests.csproj include the .cs file like this:

<Compile Include="$(CommonPath)System\IO\PathInternal.CaseSensitivity.cs" Link="Common\System\IO\PathInternal.CaseSensitivity.cs" />

and move GetIsCaseSensitiveByProbing there to get the "expected".

Something like this:

[Fact] public void IsCaseSensitiveReturnsSameValueAsProbing() { Assert.Equal(GetIsCaseSensitiveByProbing(), PathInternal.IsCaseSensitive); }

That would be unreliable test. The temp directory on Unix may live on a case insensensitive volume.

That would be unreliable test. The temp directory on Unix may live on a case insensensitive volume.

So perhaps it should be using current directory instead of temp? I know that it would still be far from perfect but I would prefer to have a single non-perfect test rather than no tests at all.

Current directory has the same problem. We can have a test like that, but it should be marked as outerloop.

We can have a test like that, but it should be marked as outerloop.

Sounds good to me.

So perhaps it should be using current directory instead of temp

That might fail on mobile for trivial reasons - I'm not sure the tests run from a writable directory. I'll give it a try and see.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Also Path.StringComparison => PathInternal.StringComparison

Move GetIsCaseSensitiveByProbing to FileSystemTest

jkotas · 2021-06-21T16:36:53Z

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs

-        /// not supported on Windows or Linux, etc. For now, this function creates a tmp file with capital letters
-        /// and then tests for its existence with lower-case letters.  This could return invalid results in corner
-        /// cases where, for example, different file systems are mounted with differing sensitivities.
-        /// </remarks>
        private static bool GetIsCaseSensitive()


This should be internal static bool IsCaseSensitive property and s_isCaseSensitive should be deleted. It is not profitable to cache it in a static, and it prevents the linker constant propagation from kicking in.

jkotas · 2021-06-21T16:38:09Z

src/libraries/System.Private.CoreLib/src/System/IO/Path.cs

@@ -957,11 +957,11 @@ private static string GetRelativePath(string relativeTo, string path, StringComp
            return sb.ToString();
        }

+        /// <summary>Gets whether the system is case-sensitive.</summary>
+        internal static bool IsCaseSensitive => PathInternal.IsCaseSensitive;


I would change the few places that use this to call PathInternal.IsCaseSensitive directly instead of introducing this wrapper.

src/libraries/System.IO.FileSystem/tests/PathInternalTests.cs

jkotas · 2021-06-21T16:41:35Z

src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs

+            }
+            catch
+            {
+                // In case something goes wrong (e.g. temp pointing to a privilieged directory), we don't


This catch made sense when it was part of the shipping code. Now that it is test, I think it would be better to delete it. We want to know when this fails as a test.

update callers to use PathInternal.IsCaseSensitive and PathInternal.StringComparison

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs

adamsitnik

LGTM, thank you @lambdageek !

src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs

lambdageek · 2021-06-22T13:29:53Z

Should we backport this to .NET 6 Preview 6? (It's not critical for MAUI on Android - they mitigated the perf hit on their end)

src/libraries/System.IO.FileSystem/tests/PathInternalTests.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub

Thanks.

stephentoub · 2021-06-22T13:38:39Z

Should we backport this to .NET 6 Preview 6?

If the perf issue was mitigated, I don't think we need to.

lambdageek · 2021-06-22T22:13:30Z

/azp run runtime-staging runtime-dev-innerloop dotnet-linker-tests

azure-pipelines · 2021-06-22T22:13:35Z

No pipelines are associated with this pull request.

…nitial_config * origin/main: (362 commits) [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300) Put Crossgen2 in sync with dotnet#54235 (dotnet#54438) Move System.Object serialization to ObjectConverter (dotnet#54436) Move setting fHasVirtualStaticMethods out of sanity check section (dotnet#54574) [wasm] Compile .bc->.o in parallel, before passing to the linker (dotnet#54053) Change PathInternal.IsCaseSensitive to a constant (dotnet#54340) Make mono_polling_required a public symbol (dotnet#54592) Respect EventSource::IsSupported setting in more codepaths (dotnet#51977) Root ComActivator for hosting (dotnet#54524) Add ILLink annotations to S.D.Common related to DbConnectionStringBuilder (dotnet#54280) Fix finalizer issue with regions (dotnet#54550) Add support for multi-arch install locations (dotnet#53763) Update library testing docs page to reduce confusion (dotnet#54324) [FileStream] handle UNC and device paths (dotnet#54483) Update NetAnalyzers version (dotnet#54511) Added runtime dependency to fix the intermittent test failures (dotnet#54587) Disable failing System.Reflection.Tests.ModuleTests.GetMethods (dotnet#54564) [wasm] Move AOT builds from `runtime-staging` to `runtime` (dotnet#54577) Keep obj node for ArrayIndex. (dotnet#54584) Disable another failing MemoryCache test (dotnet#54578) ...

lambdageek mentioned this pull request Jun 17, 2021

Issue with System.IO.GetIsCaseSensitive #54313

Closed

lambdageek added the area-System.IO label Jun 17, 2021

lambdageek added the os-android label Jun 17, 2021

marek-safar reviewed Jun 18, 2021

View reviewed changes

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.Android.cs Outdated Show resolved Hide resolved

lambdageek force-pushed the android-case-sensitive branch from 03b034c to 36740cb Compare June 18, 2021 17:34

lambdageek changed the title ~~[android] Assume filesystem is case sensitive~~ Return constants in PathInternal.GetIsCaseSensitive() on mobile platforms Jun 18, 2021

Return constants in PathInternal.GetIsCaseSensitive() on mobile platf…

c604c47

…orms In particular on Android probing using I/O is slow and contributes to slow app startup. Fixes dotnet#54339

lambdageek force-pushed the android-case-sensitive branch from 36740cb to c604c47 Compare June 18, 2021 17:38

Put OperatingSystem probing under a NET6_0_OR_GREATER define

7b8ba3e

jkotas reviewed Jun 18, 2021

View reviewed changes

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 18, 2021

View reviewed changes

lambdageek marked this pull request as draft June 19, 2021 00:36

adamsitnik reviewed Jun 21, 2021

View reviewed changes

adamsitnik added this to the 6.0.0 milestone Jun 21, 2021

adamsitnik self-assigned this Jun 21, 2021

lambdageek and others added 3 commits June 21, 2021 11:33

Apply suggested changes

6e7d107

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Implement Path.IsCaseSensitive as PathInternal.IsCaseSensitive

4ac6f7d

Also Path.StringComparison => PathInternal.StringComparison

Add test for PathInternal.IsCaseSensitive

731c90d

Move GetIsCaseSensitiveByProbing to FileSystemTest

jkotas reviewed Jun 21, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/PathInternalTests.cs Show resolved Hide resolved

jkotas reviewed Jun 21, 2021

View reviewed changes

Drop PathInternal.s_isCaseSensitive cache field

4476863

lambdageek changed the title ~~Return constants in PathInternal.GetIsCaseSensitive() on mobile platforms~~ Chase PathInternal.IsCaseSensitive to a platform-specific constant Jun 21, 2021

lambdageek changed the title ~~Chase PathInternal.IsCaseSensitive to a platform-specific constant~~ Change PathInternal.IsCaseSensitive to a platform-specific constant Jun 21, 2021

lambdageek changed the title ~~Change PathInternal.IsCaseSensitive to a platform-specific constant~~ Change PathInternal.IsCaseSensitive to a constant Jun 21, 2021

lambdageek added 2 commits June 21, 2021 13:13

Delete Path.IsCaseSensitive and Path.StringComparison

40ef8ec

update callers to use PathInternal.IsCaseSensitive and PathInternal.StringComparison

Remove catch clause from GetIsCaseSensitiveByProbing

ad2e8f2

tannergooding reviewed Jun 21, 2021

View reviewed changes

src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs Show resolved Hide resolved

lambdageek added 2 commits June 21, 2021 14:08

fixup Net5CompatTests

84e7779

Mark new test [OuterLoop]

e1098a1

lambdageek marked this pull request as ready for review June 21, 2021 19:42

adamsitnik approved these changes Jun 22, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs Outdated Show resolved Hide resolved

adamsitnik added 2 commits June 22, 2021 11:34

Apply suggestions from code review

591af35

fix formatting

b971e4b

marek-safar approved these changes Jun 22, 2021

View reviewed changes

stephentoub reviewed Jun 22, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/PathInternalTests.cs Outdated Show resolved Hide resolved

Apply suggestions

df04df4

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub approved these changes Jun 22, 2021

View reviewed changes

lambdageek removed the os-android label Jun 22, 2021

lambdageek merged commit c0ed319 into dotnet:main Jun 23, 2021

adamsitnik mentioned this pull request Jul 1, 2021

[MacCatalyst][libraries] System.IO.FileSystem.Tests failures #52857

Closed

ghost locked as resolved and limited conversation to collaborators Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change PathInternal.IsCaseSensitive to a constant #54340

Change PathInternal.IsCaseSensitive to a constant #54340

lambdageek commented Jun 17, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Jun 17, 2021

ghost commented Jun 17, 2021

jkotas commented Jun 17, 2021

marek-safar left a comment

jkotas commented Jun 18, 2021

lambdageek commented Jun 18, 2021

marek-safar commented Jun 18, 2021

lambdageek commented Jun 18, 2021 •

edited

Loading

jkotas commented Jun 18, 2021

jkotas Jun 18, 2021

jkotas Jun 18, 2021

adamsitnik Jun 21, 2021 •

edited

Loading

jkotas Jun 21, 2021

adamsitnik Jun 21, 2021

jkotas Jun 21, 2021

adamsitnik Jun 21, 2021

lambdageek Jun 21, 2021

jkotas Jun 21, 2021

jkotas Jun 21, 2021

jkotas Jun 21, 2021

adamsitnik left a comment

lambdageek commented Jun 22, 2021

stephentoub left a comment

stephentoub commented Jun 22, 2021

lambdageek commented Jun 22, 2021

azure-pipelines bot commented Jun 22, 2021

Change PathInternal.IsCaseSensitive to a constant #54340

Change PathInternal.IsCaseSensitive to a constant #54340

Conversation

lambdageek commented Jun 17, 2021 • edited Loading

dotnet-issue-labeler bot commented Jun 17, 2021

ghost commented Jun 17, 2021

jkotas commented Jun 17, 2021

marek-safar left a comment

Choose a reason for hiding this comment

jkotas commented Jun 18, 2021

lambdageek commented Jun 18, 2021

marek-safar commented Jun 18, 2021

lambdageek commented Jun 18, 2021 • edited Loading

jkotas commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

lambdageek commented Jun 22, 2021

stephentoub left a comment

Choose a reason for hiding this comment

stephentoub commented Jun 22, 2021

lambdageek commented Jun 22, 2021

azure-pipelines bot commented Jun 22, 2021

lambdageek commented Jun 17, 2021 •

edited

Loading

lambdageek commented Jun 18, 2021 •

edited

Loading

adamsitnik Jun 21, 2021 •

edited

Loading