Remove ternary workaround for string.IsNullOrEmpty #63095
Conversation
|
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
added
the
community-contribution
huoyaoyuan
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsCloses #4207 Codegen for minimal sample is recorded in #4207 (comment) . How to confirm it in larger range?
|
If PR is closing the issue, then this workaround should be removed from all places? $ git grep -l 'github.*4207'
src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs
src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs
src/libraries/System.Private.CoreLib/src/System/DateTime.cs
src/libraries/System.Private.CoreLib/src/System/Delegate.cs
src/libraries/System.Private.CoreLib/src/System/Globalization/Ordinal.cs
src/libraries/System.Private.CoreLib/src/System/Globalization/SortVersion.cs
src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInfo.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/EventInfo.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/FieldInfo.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/MemberInfo.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInfo.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/Module.cs
src/libraries/System.Private.CoreLib/src/System/Reflection/PropertyInfo.cs
src/libraries/System.Private.CoreLib/src/System/String.cs
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Helpers.cs
src/libraries/System.Private.CoreLib/src/System/Version.cs
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ContinuationTests.NullToken.cs |
It should be. I don't mentioned that the workaround is used so many places. How to view the codegen diff? |
One way would be to use jitutils, by collecting the FWIW, I agree that this pattern should no longer be necessary after #61023 and #61275. |
With runtime checked build ( |
|
given: public bool M1(Delegate? f) {
return f is null;
}
public bool M2(Delegate? f) {
return f is null ? true : false;
}It used to be the case that codegen was better for M2 than M1, but now it is the opposite. I think the expectation at some point was to get identical codegen so perhaps #4207 still needs some work? |
|
There are more places if I search for |
It used to be always situational, in some cases ? true : false helped, in some made codegen worse (after inlining of those M1 and M2) |
|
jit-diff for String.IsNullOrEmpty change only: |
|
Why |
Hard to say, somehow due to that change it wasn't inlined into |
|
I confirmed that the patterns where |
I don't have that locally anymore, you can run it like this: Next step is to clone & build https://github.com/dotnet/jitutils Then do: When we work on jit we usually just use |
|
JIT diff of current commits:
```
Summary of Code Size diffs:
(Lower is better)
Total bytes of base: 62820381 Top file regressions (bytes): Top file improvements (bytes): 138 total files with Code Size differences (22 improved, 116 regressed), 135 unchanged. Top method regressions (bytes): Top method improvements (bytes): Top method regressions (percentages): Top method improvements (percentages): 1667 total methods with Code Size differences (436 improved, 1231 regressed), 277338 unchanged. |
huoyaoyuan
force-pushed
the
string-isnullorempty
branch
from
fd7e8fc
to
e3937c7
Compare
|
I'm confused by the results I'm getting. I'm comparing the first commit (IsNullOrEmpty change) only. The commits are @EgorBo There's a much larger influenced set than your result. I checked out them into 2 worktrees, and deleted the |
|
@huoyaoyuan can you replace |
|
OK with crossgen I'm getting much more reasonable result: |
|
@EgorBo could you help me to analyze the change? When I try to analyze impact of the second commit, the result shows a lot of |
|
@huoyaoyuan sorry for the delay, From my understanding we need #63720 to check in and, potentially, one more PR to basically convert |
See https://gist.github.com/EgorBo/a72bfa70f08a4d890241037c0fc1fb93, replace --corelib with -f |
|
I used the method from your previous comment:
|
so what is the diff for you changes from those steps? |
|
I ran it locally for only IsNullOrEmpty part and it was way more verbose than what you posted previously |
|
The result is already with |
can you paste the results, full report with the header how many improved/regressed - I'm pretty sure #63095 (comment) it's not |
|
Here is the full command and result:
PS D:\Code\dotnet\jitutils\bin> .\jit-analyze.exe -b D:\Code\dotnet\jitdiff\dasmset_1\base\ -d D:\Code\dotnet\jitdiff\dasmset_3\base\ -c 100
Found 273 files with textual diffs.
Summary of Code Size diffs: Total bytes of base: 37323903 Top file regressions (bytes): 1 total files with Code Size differences (0 improved, 1 regressed), 271 unchanged. Top method regressions (bytes): Top method improvements (bytes): Top method regressions (percentages): Top method improvements (percentages): 112 total methods with Code Size differences (59 improved, 53 regressed), 254791 unchanged. |
|
Thanks, lgtm, let's wait for a green CI |
src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs
Outdated
Show resolved
Hide resolved
|
@EgorBo CI is green now. Does it require another person to merge this? |
| if (typeof(T) == typeof(double)) return (double)(object)left < (double)(object)right ? true : false; | ||
| if (typeof(T) == typeof(Half)) return (Half)(object)left < (Half)(object)right ? true : false; | ||
| return left.CompareTo(right) < 0 ? true : false; | ||
| if (typeof(T) == typeof(byte)) return (byte)(object)left < (byte)(object)right; |
There was a problem hiding this comment.
why these if’s and not using switch expressions for these in this file?
There was a problem hiding this comment.
The IL patterns of typeof(T) == constant and (the same type)(object)value are specially recognized by JIT and will be reduced to a nop. Other patterns are harder to analysis.
There was a problem hiding this comment.
Also, typeof() isn't a constant value and can't be used in a switch statement/expression.
|
Also note: With rider you can have the cursor in the method for which you want it to give the JIT asm on and works even without compiling. If only Visual Studio had something similar built in as an tab one can click on to view it the same way as well. |
|
https://github.com/EgorBo/Disasmo is useful. |
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2) | ||
| { | ||
| // Test d2 first to allow branch elimination when inlined for null checks (== null) | ||
| // so it can become a simple test | ||
| if (d2 is null) | ||
| { | ||
| // return true/false not the test result https://github.com/dotnet/runtime/issues/4207 | ||
| return (d1 is null) ? true : false; | ||
| return d1 is null; | ||
| } | ||
|
|
||
| return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1); |
There was a problem hiding this comment.
| return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1); | |
| return ReferenceEquals(d2, d1) || d2.Equals((object?)d1); |
There was a problem hiding this comment.
This is quite a similar pattern to the one addressed by this PR. It's better to confirm there's no codegen regression separately.
| @@ -453,8 +450,7 @@ public sealed override Delegate[] GetInvocationList() | |||
| // so it can become a simple test | |||
| if (d2 is null) | |||
| { | |||
| // return true/false not the test result https://github.com/dotnet/runtime/issues/4207 | |||
| return (d1 is null) ? false : true; | |||
| return d1 is not null; | |||
| } | |||
|
|
|||
| return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1); | |||
There was a problem hiding this comment.
| return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1); | |
| return !ReferenceEquals(d2, d1) && !d2.Equals(d1); |
|
@huoyaoyuan thanks! |
|
Possible regression on x86: #63095 |
|
So the issue may not be fully fixed for all platforms. Ah. |
Closes #4207
Codegen for minimal sample is recorded in #4207 (comment) . How to confirm it in larger range?