Optimize is null #18657

TyOverby · 2017-04-12T23:31:33Z

optimizes obj is null patterns.

Customer scenario

When the customer gets faster obj is null patterns, resulting in prettier code where before they had to write (object) obj == null or obj.HasValue for similar performance.

Bugs this fixes:

#13247

Workarounds, if any

The old behavior of explicitly writing out method calls still works.

Risk

Low

Performance impact

Perf in the compiler is unaffected but programs compiled with this change will potentially get faster.

Is this a regression from a previous update?

No

Root cause analysis:

This optimization was not added earlier due to time constraints.

How was the bug found?

This was a known "want to have" feature from earlier in the development cycle.

TyOverby · 2017-04-12T23:31:45Z

@gafter

TyOverby · 2017-04-12T23:31:54Z

@dotnet/roslyn-compiler

TyOverby · 2017-04-12T23:32:24Z

I'll add a few "switch/case" pattern unit tests tomorrow.

TyOverby · 2017-04-12T23:34:00Z

Also, this is likely to fail existing tests since I never checked to see that they would work.

jaredpar

Really hope we can take this change. But unsure if there is some subtle reason why we have to emit code using Equals instead of direct null comparison. @gafter will be able to clear up if this is a legal optimization or not.

jaredpar · 2017-04-13T00:55:00Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), boundConstant),
-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), input)
-                );
+            var systemObject = _factory.SpecialType(SpecialType.System_Object);


Meta feedback: it's really unclear where in the compiler we can, or can't, depend on special types being non-null in the compiler. Depending on it in the local rewriter seems sane, but I've been surprised in the past. Can we take a follow up item to get good guidance on this point?

The pattern-matching is operator is very restricted in the kinds of conversions it can apply to the left operand. There is no target type for the left operand. It is essentially converted to object (by spec) only to bind to the second operand of static bool object.Equals(object, object). Thus only a boxing conversion could change its representation, and a boxing conversion never produces null (except from a nullable type).

Looking again, perhaps because the pattern is lowered that the constant is always a literal. If that is the case, you shouldn't have to test for it.

jaredpar · 2017-04-13T00:57:35Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), input)
-                );
+            var systemObject = _factory.SpecialType(SpecialType.System_Object);
+            if (boundConstant is BoundLiteral bl && bl.ConstantValue.IsNull)


I'm not sure we can do this in every single case the RHS is null. One case that cames to mind is when the LHS is an unconstrained type parameter. Those can be compared to null using ==. I'm unsure if that's allowed with is though.

Two problems with this code. First, you should not assume that a literal has a non-null constant value. Second, you shouldn't require that the form of the constant value is a literal. I suggest if (boundConstant.ConstantValue?.IsNull == true)

Looking again, perhaps because the pattern is lowered that the constant is always a literal. If that is the case, you shouldn't have to test for it.

gafter · 2017-04-13T18:41:38Z

Fixes #13247.

gafter · 2017-04-13T18:45:58Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), boundConstant),
-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), input)
-                );
+            var systemObject = _factory.SpecialType(SpecialType.System_Object);


The pattern-matching is operator is very restricted in the kinds of conversions it can apply to the left operand. There is no target type for the left operand. It is essentially converted to object (by spec) only to bind to the second operand of static bool object.Equals(object, object). Thus only a boxing conversion could change its representation, and a boxing conversion never produces null (except from a nullable type).

gafter · 2017-04-13T18:48:01Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), input)
-                );
+            var systemObject = _factory.SpecialType(SpecialType.System_Object);
+            if (boundConstant is BoundLiteral bl && bl.ConstantValue.IsNull)


Two problems with this code. First, you should not assume that a literal has a non-null constant value. Second, you shouldn't require that the form of the constant value is a literal. I suggest if (boundConstant.ConstantValue?.IsNull == true)

gafter · 2017-04-13T18:54:42Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

+                return _factory.Binary(
+                    kind: BinaryOperatorKind.Equal,
+                    type: systemBoolean,
+                    left: _factory.Convert(systemObject, input),


Probably not the best way to test for null when the value is of a value type or a nullable value type. In that case this code still boxes unnecessarily. You may be able to invoke the helper RewriteNullableNullEquality to get the desired effect. If you elect not to fix that in this PR, please note in #18657 that the case still remains unoptimized.

I think this should call MakeBinaryOperator, which should handle this kind of optimization and deal with lifted compares.
Just be careful with what is passed as operatorKind and method.

I think you can have here:
BinaryOperatorKind.NullableNullEqual
BinaryOperatorKind.ObjectEquals
BinaryOperatorKind.OperatorMethodEquals (with Object.Equals as method)

gafter · 2017-04-13T18:59:08Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

+            if (boundConstant is BoundLiteral bl && bl.ConstantValue.IsNull)
+            {
+                var systemBoolean = _factory.SpecialType(SpecialType.System_Boolean);
+                return _factory.Binary(


_factory.ObjectEqual(_factory.Convert(systemObject, input), right)

gafter · 2017-04-24T19:23:49Z

@TyOverby What is your status on this PR?

TyOverby · 2017-04-24T19:44:33Z

@gafter it's on my todo list, but not high enough for anything progress to be made while async-main hasn't landed.

gafter · 2017-04-25T03:38:52Z

@TyOverby Makes sense to me, thanks.

TyOverby · 2017-05-22T21:41:42Z

@gafter: I made the changes that you recommended

@dotnet/roslyn-compiler Out for review.

jaredpar · 2017-05-24T19:32:00Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenOperators.cs

+  IL_0010:  nop
+  IL_0011:  ret
+}");
+        }


Blank line after closing }

jaredpar · 2017-05-24T19:32:41Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenOperators.cs

@@ -13,6 +13,50 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
 {
    public class CodeGenOperatorTests : CSharpTestBase
    {
+
+        [Fact]
+        public void TestIsNullPattern()


Other tests that we should add:

Value of unconstrained type parameter testing is null

Value of type parameter constrained to class against is null

A constant, which is null, testing against is null

Also add couple tests were "is" is used as a condition - like:

if (c is null) . . .

In reply to: 118344562 [](ancestors = 118344562)

VSadov · 2017-05-24T20:24:18Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

+                        loweredRight: boundConstant,
+                        returnType: systemBoolean
+
+                    );


extra space.

TyOverby · 2017-05-25T16:56:59Z

@jaredpar @VSadov: I've added the recommended tests.

VSadov

LGTM

jaredpar

Pending @gafter sign off.

TyOverby · 2017-05-25T18:39:46Z

@dotnet-bot test windows_release_unit64_prtest please

TyOverby · 2017-05-25T20:17:55Z

@gafter Any more recommendations?

gafter · 2017-05-26T17:45:50Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

-                _factory.Convert(_factory.SpecialType(SpecialType.System_Object), input)
-                );
+            var systemObject = _factory.SpecialType(SpecialType.System_Object);
+            if (boundConstant is BoundLiteral bl && bl.ConstantValue?.IsNull == true)


The BoundLiteral test is incorrect. This should only depend on whether the constant value is null (whether or not it is a literal). For example, the right-hand-side could be a named constant. And we should have a test for that scenario (with type string).

TyOverby · 2017-05-26T22:43:15Z

@gafter Done. Also added a test like you suggested.

gafter

LGTM

TyOverby · 2017-05-30T16:08:19Z

@gafter It occurs to me that this code doesn't handle Nullable<T>. I'm going to try to add this functionality and add tests.

TyOverby · 2017-05-30T16:48:23Z

~~@gafter @VSadov: I've added similar optimizations for objects that are of type Nullable<T>. Please re-review this commit at your convenience.~~

This reverts commit 6dd7452.

TyOverby · 2017-05-30T16:58:19Z

reverted the most recent change due to signoff issues. I'll make a new PR for that functionality.

TyOverby · 2017-05-30T17:04:27Z

Ping @MeiChin-Tsai for approval.

MeiChin-Tsai · 2017-05-30T17:14:42Z

Sorry to miss this earlier. Since the risk is low, we will take it. This is really consider "a small feature", correct?

TyOverby · 2017-05-30T17:42:29Z

@MeiChin-Tsai: yeah, risk is low. I wouldn't even consider this a feature, more like a "perf fix"

MeiChin-Tsai · 2017-05-30T17:44:14Z

thx for clarification, @TyOverby. Approved.

Optimize is null

dnfclas added the cla-already-signed label Apr 12, 2017

jaredpar reviewed Apr 13, 2017

View reviewed changes

gafter suggested changes Apr 13, 2017

View reviewed changes

Pilchie added the Area-Compilers label Apr 14, 2017

gafter assigned TyOverby Apr 18, 2017

jaredpar added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 1, 2017

TyOverby force-pushed the optimize-is-null branch 2 times, most recently from 39a848a to 5e02454 Compare May 22, 2017 21:32

optimize "is null" patterns

090844e

TyOverby force-pushed the optimize-is-null branch from 5e02454 to 090844e Compare May 22, 2017 21:32

TyOverby removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 22, 2017

jaredpar reviewed May 24, 2017

View reviewed changes

VSadov reviewed May 24, 2017

View reviewed changes

add more tests

f386eb8

VSadov approved these changes May 25, 2017

View reviewed changes

jaredpar reviewed May 25, 2017

View reviewed changes

gafter suggested changes May 26, 2017

View reviewed changes

use different method for seeing if something is a constant

ad41667

gafter approved these changes May 30, 2017

View reviewed changes

optimize is_null on nullable<t>

6dd7452

Revert "optimize is_null on nullable<t>"

a267219

This reverts commit 6dd7452.

TyOverby merged commit cb9ee8c into dotnet:master May 30, 2017

AdamSpeight2008 pushed a commit to AdamSpeight2008/roslyn-AdamSpeight2008 that referenced this pull request Jun 4, 2017

Merge pull request dotnet#18657 from TyOverby/optimize-is-null

078cfb0

Optimize is null

Optimize is null #18657

Optimize is null #18657

Uh oh!

Conversation

TyOverby commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TyOverby commented Apr 12, 2017

Uh oh!

TyOverby commented Apr 12, 2017

Uh oh!

TyOverby commented Apr 12, 2017

Uh oh!

TyOverby commented Apr 12, 2017

Uh oh!

jaredpar 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!

gafter Apr 13, 2017 • 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!

gafter commented Apr 13, 2017

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!

gafter commented Apr 24, 2017

Uh oh!

TyOverby commented Apr 24, 2017

Uh oh!

gafter commented Apr 25, 2017

Uh oh!

TyOverby commented May 22, 2017

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!

TyOverby commented May 25, 2017

Uh oh!

VSadov left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

TyOverby commented May 25, 2017

Uh oh!

TyOverby commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TyOverby commented Apr 12, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading

TyOverby commented May 30, 2017 •

edited

Loading