Faster null checks #21736

benaadams · 2019-01-01T17:26:34Z

operator ==(MethodInfo left, MethodInfo right) was showing up in traces, ~40% of them from == null checks

This changes the null checks against (most) MemberInfo derived classes in coreclr to be an is null fast null check and related changes; rather than calling through to their operator overloads to test for null.

Mostly effects class types that overload the operator== and operator!= as these methods are then used for null.

Pre

Post

jkotas · 2019-01-01T18:55:55Z

src/System.Private.CoreLib/shared/System/Reflection/MemberInfo.cs

                return false;
+            }
+
+            if (left is Type type1)


Should this rather call left.Equals(right) instead of this manual dispatch?

Looks like Type is the only particularly special one where operator == is an external call and its .Equals test is a ReferenceEquals; so maybe have to manually dispatch for Type?

operator== is external call on Type just so the JIT can recognize it as intrinsic. It is from the days when the method had to be FCall in order to be recognized as intrinsic.

It should be fine to just call Equals for Type as well.

src/System.Private.CoreLib/shared/System/Reflection/MethodBase.cs

jkotas · 2019-01-01T19:02:14Z

Nice! Have you done anything to find these automatically - should we do the same cleanup in other repos too?

benaadams · 2019-01-01T22:46:11Z

Have you done anything to find these automatically

I'd like to say I have... that would, sound good 😅

benaadams · 2019-01-01T23:02:24Z

Commit c0884ae is for isnot null checks t is object; because !(t is null) is ugly and C# doesn't have a better semantic.

Should be easier to search and replace the simple is object
than replacing the regex \!\(([a-zA-Z\[\]]+) is null\) when it does.

/cc @jaredpar

benaadams · 2019-01-02T01:42:27Z

Going to go all in on this

jkotas · 2019-01-02T02:25:45Z

t is object

This construct does not look anything like a null check. !(x is null) is more readable. I would optimize for readability at any given point, not for future find&replace that may or may not happen.

jkotas · 2019-01-02T13:19:26Z

cc @stephentoub Do you have an opinion about doing this globally and about the pattern to use for the "is not null" case?

gfoidl

Not all changes are necessary, as the code gets less readable and the C# compiler emits the same code. See the two examples I've commented.

For the initial motivation with MethodInfo there is a difference, notably by casting to object and then performing the null-check instead of the (not inlined) method call.

gfoidl · 2019-01-02T13:20:16Z

src/System.Private.CoreLib/shared/Internal/IO/File.cs

            try
            {
-                if (path == null)
+                if (path is null)


Is here actually a difference as Roslyn generates the same code for both?

BTW: here I would use string.IsNullOrEmpty for this and the following check.

gfoidl · 2019-01-02T13:21:01Z

src/System.Private.CoreLib/Common/System/SR.cs

                _currentlyLoading.Add(key); // Push

-                if (ResourceManager == null)
+                if (ResourceManager is null)


Is here actually a difference as Roslyn generates the same code for both?

Though I find this version more readable / nicer 😉.

Can either pick consistency or ease of changing 4k+ call sites 😄

Early commits I was only picking a specific set of classes; doing it for the all == null and != null checks is either much slower or more broad brush (or would need time to write a tool)

That's right 😉

!(obj is null) vs. obj != null the latter is more readable.
obj is null vs obj == null I find both very readable.
So it's hard to find a good balance between consistency and readability, as for MethodInfo and the like the is-variant has to be taken perf-wise -- or adapt the compiler(s) to emit efficient code for the null-comparison.

Was a discussion (on twitter) about a less fugly is not null check for C#; most popular variants seemed to be x !is null and x is !null

Don't know if there is a formal C# proposal though

VB's leading the way here by already having IsNot Nothing 😉

Too many channels to follow 😃 Thanks for the link!

formal proposal

It's there: dotnet/csharplang#27. Someone suggested aint operator as well. 😄

I think this is the current proposal that would end up landing the 'not' pattern: dotnet/csharplang#1350

For checking not null specifically, there's also the syntax that falls out from Property Patterns: if (foo is {}), though I will concede that it's not quite as readable as if (foo is not null), but the outcome is the exact same.

if (foo is object) also works, but undid that in e4389b0 as it wasn't very clear

benaadams · 2019-01-02T16:16:52Z

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test

benaadams · 2019-01-02T16:17:35Z

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

stephentoub · 2019-01-02T18:53:48Z

@stephentoub Do you have an opinion about doing this globally and about the pattern to use for the "is not null" case?

If we're going to do it, we should do it everywhere / globally, across coreclr/corert/corefx, for both the is and is not cases. For is not, we should go with !(foo is null) until there's a better option (which foo is object is not 😄). There's unfortunately going to continue to be inconsistency even when we do that, in that == checks will still be used for comparisons other than explicit nulls, null comparisons are performed via other means like Interlocked.CompareExchange, etc.

The other option would be to never use foo is null, and instead explicitly use ReferenceEquals(foo, null) when we want to avoid == overloaded semantics. I don't have a strong opinion; I'm ok with the is null option.

benaadams · 2019-01-02T19:32:09Z

To improve the common use of right-sided null x == null upstream.

If we trim most of the class operator== methods to only null check (some also ReferenceEquals and check either arg for null) then switch the common order of the params used for .Equals (which is normally left.Equals(right); so:

public static bool operator== (MyClass left, MyClass right)
    => (right is null) ? (left is null) : right.Equals(left);

Then it drops under the [below ALWAYS_INLINE size] inlining threshold; after inlining the Jit can remove the branch (as right is constant null) so it becomes just left is null

jkotas · 2019-01-02T19:40:29Z

If we trim most of the class operator== methods to only null check (some also ReferenceEquals and check either arg for null) then switch the common order of the params used for .Equals

So would the whole perf benefit of this change come from doing just this?

benaadams · 2019-01-02T19:52:58Z

So would the whole perf benefit of this change come from doing just this?

Just checking

galich · 2019-01-02T19:57:46Z

just 2 cents - i'd rather see compiler improved to support this x==null case instead of everyone changing their code

benaadams · 2019-01-02T20:20:22Z

So would the whole perf benefit of this change come from doing just this?

Works even better; every call evaporates even the non null tests #21765

lothrop · 2019-01-03T14:33:25Z

src/System.Private.CoreLib/src/System/MulticastDelegate.cs

-            if (v == null)
+            if (!(value is MulticastDelegate v))
                return this;
            if (v._invocationList as object[] == null)


Why is == null used here?

lothrop · 2019-01-03T14:34:39Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

+            if (argumentType is null)
                throw new ArgumentNullException(nameof(argumentType));

            m_value = (value == null) ? null : CanonicalizeValue(value);


Should is be used here?

jkotas reviewed Jan 1, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Reflection/MethodBase.cs Outdated Show resolved Hide resolved

benaadams changed the title ~~Faster null checks~~ [WIP] Faster null checks Jan 2, 2019

benaadams added 6 commits January 2, 2019 12:51

Faster null checks

d3ae0c3

Feedback

ddb03ff

isnot null

23693ef

Some more

d85c9c5

object casts

0ed3baf

Go all in

d9ef0ec

benaadams force-pushed the null-checks branch from a870499 to d2bd332 Compare January 2, 2019 12:53

benaadams changed the title ~~[WIP] Faster null checks~~ Faster null checks Jan 2, 2019

benaadams force-pushed the null-checks branch from d2bd332 to e4389b0 Compare January 2, 2019 13:13

x is object => !(x is null)

e4389b0

gfoidl reviewed Jan 2, 2019

View reviewed changes

search replace error

5bcb6a5

benaadams closed this Jan 2, 2019

benaadams mentioned this pull request Jan 2, 2019

Support faster null checks #21765

Merged

benaadams deleted the null-checks branch January 3, 2019 10:58

lothrop reviewed Jan 3, 2019

View reviewed changes

Faster null checks #21736

Faster null checks #21736

Uh oh!

Conversation

benaadams commented Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

jkotas commented Jan 1, 2019

Uh oh!

benaadams commented Jan 1, 2019

Uh oh!

benaadams commented Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Jan 2, 2019

Uh oh!

jkotas commented Jan 2, 2019

Uh oh!

jkotas commented Jan 2, 2019

Uh oh!

gfoidl left a comment

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!

benaadams Jan 2, 2019 • 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!

Joe4evr Jan 2, 2019 • 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!

benaadams commented Jan 2, 2019

Uh oh!

benaadams commented Jan 2, 2019

Uh oh!

stephentoub commented Jan 2, 2019

Uh oh!

benaadams commented Jan 2, 2019

Uh oh!

jkotas commented Jan 2, 2019

Uh oh!

benaadams commented Jan 2, 2019

Uh oh!

galich commented Jan 2, 2019

Uh oh!

benaadams commented Jan 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

benaadams commented Jan 1, 2019 •

edited

Loading

benaadams commented Jan 1, 2019 •

edited

Loading

benaadams Jan 2, 2019 •

edited

Loading

Joe4evr Jan 2, 2019 •

edited

Loading