Fix ascii range check by benaadams · Pull Request #16535 · dotnet/coreclr

benaadams · 2018-02-23T23:50:44Z

benaadams · 2018-02-24T00:01:46Z


                // in InvariantMode we support all range and not only the ascii characters.
-                char maxChar = (char) (GlobalizationMode.Invariant ? 0xFFFF : 0x80);
+                char maxChar = (char) (GlobalizationMode.Invariant ? 0xFFFF : 0x7f);


Is GlobalizationMode.Invariant initrinsicish to the Jit? (like readonly static) and will it branch eliminate?

should use _invariantMode and not GlobalizationMode.Invariant

nit: Change 0x7f to 0x7F (all capital) to be consistent.

jkotas · 2018-02-24T00:33:05Z

-        // _invariantMode is defined for the perf reason as accessing the instance field is faster than access the static property GlobalizationMode.Invariant
+        // s_invariantMode is defined for the perf reason as accessing the field is faster than access the static property GlobalizationMode.Invariant
        [NonSerialized]
-        private readonly bool _invariantMode = GlobalizationMode.Invariant;


GlobalizationMode.Invariat is a readonly static already. We do not need to duplicate it.

This is cached in a instance field because instance field access is faster (in crossgened code in particular - and CoreLib is pretty much always crossgened).

right, we just need to replace GlobalizationMode.Invariant with _invariantMode in CompareInfo code.

All the instance methods already use it; other ones are static

ahsonkhan · 2018-02-24T00:35:35Z

-    {
+    {   
+        // s_invariantMode is defined for the perf reason as accessing the field is faster than access the static property GlobalizationMode.Invariant
+        private static readonly bool s_invariantMode = GlobalizationMode.Invariant;


See: #16379 (comment)

This cache does not make sense. GlobalizationMode.Invariant property should have the exact same perf as you local cache.

ahsonkhan · 2018-02-24T00:40:52Z

Can you also please update this comment?

coreclr/src/mscorlib/src/System/String.Comparison.cs

Line 68 in cb4dd60

    
           //This will not work in case-insensitive mode for any character greater than 0x80.

Thanks for fixing this.

benaadams · 2018-02-24T01:02:17Z

Addressed feedback

tarekgh · 2018-02-24T01:06:45Z

Thanks @benaadams

benaadams · 2018-02-24T05:37:02Z

@dotnet-bot Tizen armel Cross Checked Innerloop Build and Test

benaadams · 2018-02-24T05:40:27Z

Won't let me rerun the Tizen

danmoseley · 2018-02-24T05:55:16Z


                // in InvariantMode we support all range and not only the ascii characters.
-                char maxChar = (char) (GlobalizationMode.Invariant ? 0xFFFF : 0x80);
+                char maxChar = (char) (GlobalizationMode.Invariant ? 0xFFFF : 0x7F);


Could this be computed just once in a static ctor? It's cheap, but this code is called a lot. (I can't see the context right now so this may be off.)

@benaadams, thoughts?

@jkotas pointed out it already is #16535 (comment)

I was referring to the value, not GlobalizationMode. Ie put either 0xFFFF or 0x7F in a readonly static field. But seems unlikely that would be any faster than a conditional check each time here.

ahsonkhan · 2018-02-24T06:13:08Z

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test

ahsonkhan · 2018-02-24T06:13:34Z

Won't let me rerun the Tizen

Must have been some temporary issue.

stephentoub · 2018-02-24T11:34:10Z

Sounds like we're also missing a test in corefx?

benaadams · 2018-02-24T12:49:21Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

benaadams · 2018-02-24T17:02:47Z

Path failures

d7295605\_\fx\src\System.Runtime.Extensions\tests\System\IO\PathTests.cs(759,0): at System.IO.Tests.PathTests.GetFullPath_Windows_NotSupportedExceptionPaths(String path)
06:02:23      System.IO.Tests.PathTests.GetFullPath_Windows_UNC_Valid(expected: \"\\\\\\\\LOCALHOST\\\\share\\\\test.txt.~SS\", input: \"\\\\\\\\LOCALHOST\\\\share\\\\test.txt.~SS\") [FAIL]
06:02:23         Assert.Equal() Failure
06:02:23                                          � (pos 30)
06:02:23         Expected: úúúT\share\test.txt.~SS
06:02:23         Actual:   úúúT\share\test.txt.~SS\0
06:02:23                                          � (pos 30)

ahsonkhan · 2018-02-24T17:46:29Z

From Tizen armel Cross Checked Innerloop:
https://ci.dot.net/job/dotnet_coreclr/job/master/job/armel_cross_checked_tizen_innerloop_prtest/1065/consoleFull#9887747564b7730ef-150b-4be7-a00f-82ba6c50792b

23:06:07 FAILED   - JIT/Regression/CLR-x86-JIT/V1-M09.5-PDC/b29068/b29068/b29068.sh
23:06:07                BEGIN EXECUTION
23:06:07                /home/coreclr/bin/tests/Windows_NT.x64.Checked/Tests/coreoverlay/corerun b29068.exe
23:06:07                qemu: Unsupported syscall: 389
23:06:07 
               ./b29068.sh: line 243:   905 Segmentation fault      (core dumped) $_DebuggerFullPath "$CORE_ROOT/corerun" $ExePath $CLRTestExecutionArguments
23:06:07                Expected: 100
23:06:07                Actual: 139
23:06:07                END EXECUTION - FAILED

cc @AndyAyersMS

benaadams · 2018-02-24T17:51:41Z

qemu: Unsupported syscall is https://github.com/dotnet/coreclr/issues/12972

ahsonkhan · 2018-02-24T17:58:12Z

Path failures

@JeremyKuhne, @danmosemsft
I think this test failure is caused by the recent changes to to PathHelper.Normalize here: #16478 / https://github.com/dotnet/coreclr/pull/16512/files#diff-8ba815a999fa742edfe2266080bb7804

As an aside, this issue within the comments has been closed. Should it be removed?
https://github.com/dotnet/corefx/blob/92afa6ec21817739ebf45cd66b4569a03b54d4c0/src/System.Runtime.Extensions/tests/System/IO/PathTests.cs#L928

ahsonkhan · 2018-02-24T18:06:31Z

Sounds like we're also missing a test in corefx?

Yes, I think so, We may be missing a test case on the boundary between 0x7F and 0x80 where we aren't calling into the OS when we should. I wonder if the results are any different though.

AndyAyersMS · 2018-02-24T18:14:44Z

I though the plan was to decommission the Tizen tests as they are flaky. @RussKeldorph ??

JeremyKuhne · 2018-02-24T18:23:06Z

@ahsonkhan, @benaadams Go ahead and disable the path test against me and I'll fix it asap- only ~ paths go down this code path. If you see any failures without a ~ in the path let me know. (Note that for some reason the PathTests class isn't executing correctly, which is why I missed this. It appears that after a certain number of test methods tests get skipped silently. Full investigation is ongoing.)

RussKeldorph · 2018-02-26T16:43:38Z

Re: Tizen: The plan is to remove the emulator-based Tizen testing from default PR-triggers, due to the aforementioned flakiness, after we have reliable hardware-based ARM Linux testing in PR, which @BruceForstall is working on.

ahsonkhan · 2018-02-26T20:42:01Z

Go ahead and disable the path test against me and I'll fix it asap- only ~ paths go down this code path. If you see any failures without a ~ in the path let me know. (Note that for some reason the PathTests class isn't executing correctly, which is why I missed this. It appears that after a certain number of test methods tests get skipped silently. Full investigation is ongoing.)

@JeremyKuhne, does this PR resolve the failing tests were are seeing here, or do we still need to disable some tests?
dotnet/corefx#27449

ahsonkhan · 2018-02-26T20:45:20Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

ahsonkhan · 2018-02-26T23:07:31Z

CI is passing (modulo Tizen), is this good to merge?

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

benaadams mentioned this pull request Feb 23, 2018

Inlinable String.Equals for ref equality #16434

Closed

benaadams commented Feb 24, 2018

View reviewed changes

jkotas reviewed Feb 24, 2018

View reviewed changes

ahsonkhan reviewed Feb 24, 2018

View reviewed changes

benaadams force-pushed the ascii-range branch from 5d1d349 to 5671023 Compare February 24, 2018 00:55

Fix ascii range check

be586ab

benaadams force-pushed the ascii-range branch from 5671023 to be586ab Compare February 24, 2018 01:00

tarekgh approved these changes Feb 24, 2018

View reviewed changes

ahsonkhan approved these changes Feb 24, 2018

View reviewed changes

danmoseley reviewed Feb 24, 2018

View reviewed changes

ahsonkhan merged commit 47c39ed into dotnet:master Feb 27, 2018

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Feb 27, 2018

Fix ascii range check (dotnet/coreclr#16535)

8c163eb

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Feb 27, 2018

Fix ascii range check (dotnet/coreclr#16535)

2d9d4d9

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corefx that referenced this pull request Feb 27, 2018

Fix ascii range check (dotnet/coreclr#16535)

bfc490d

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corert that referenced this pull request Feb 27, 2018

Fix ascii range check (dotnet/coreclr#16535)

35d4e5f

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

kbaladurin pushed a commit to kbaladurin/corert that referenced this pull request Mar 15, 2018

Fix ascii range check (dotnet/coreclr#16535)

71b2774

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

okodron mentioned this pull request Mar 16, 2018

Add environment variable (COMPlus_EnableDiagnostics) to disable debuggging and profiling #16141

Merged

Conversation

benaadams commented Feb 23, 2018

Uh oh!

benaadams Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Feb 24, 2018

Uh oh!

tarekgh commented Feb 24, 2018

Uh oh!

benaadams commented Feb 24, 2018

Uh oh!

benaadams commented Feb 24, 2018

Uh oh!

danmoseley Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Feb 24, 2018

Uh oh!

ahsonkhan commented Feb 24, 2018

Uh oh!

stephentoub commented Feb 24, 2018

Uh oh!

benaadams commented Feb 24, 2018

Uh oh!

benaadams commented Feb 24, 2018

Uh oh!

ahsonkhan commented Feb 24, 2018

Uh oh!

benaadams commented Feb 24, 2018

Uh oh!

ahsonkhan commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan commented Feb 24, 2018

Uh oh!

AndyAyersMS commented Feb 24, 2018

Uh oh!

JeremyKuhne commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussKeldorph commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan commented Feb 26, 2018

Uh oh!

ahsonkhan commented Feb 26, 2018

Uh oh!

ahsonkhan commented Feb 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

benaadams Feb 24, 2018 •

edited

Loading

ahsonkhan commented Feb 24, 2018 •

edited

Loading

danmoseley Feb 24, 2018 •

edited

Loading

ahsonkhan commented Feb 24, 2018 •

edited

Loading

JeremyKuhne commented Feb 24, 2018 •

edited

Loading

RussKeldorph commented Feb 26, 2018 •

edited

Loading