Tuple equality: Optimize cases where nullable always has value by jcouv · Pull Request #25644 · dotnet/roslyn

jcouv · 2018-03-21T04:50:24Z

Customer scenario

Optimize the tuple-equality case where a nullable tuple or nullable element is known to never be null.

Bugs this fixes

Fixes #25487

Risk

Performance impact

Low. The change is only affecting tuple equality lowering.

Is this a regression from a previous update?

No

jcouv · 2018-03-21T23:02:45Z

@AlekseyTs @dotnet/roslyn-compiler for review. This is a follow-up optimization raised during the main code review for tuple equality (#23356 (comment), #23356 (review))
Thanks #Resolved

AlekseyTs · 2018-03-21T23:57:56Z

            // in `(..., expr) == (..., (..., ...))` we need to save `expr` because it is used in a simple comparison
-            return EvaluateSideEffectingArgumentToTemp(VisitExpression(expr), initEffects, ref temps);
+            BoundExpression loweredExpr = VisitExpression(expr);
+            if ((bool)(loweredExpr.Type != null) && NullableAlwaysHasValue((BoundExpression)loweredExpr) is var value && value != null)


(bool) [](start = 16, length = 6)

Not sure why do we need the cast to bool here #Closed

AlekseyTs · 2018-03-21T23:58:10Z

            // in `(..., expr) == (..., (..., ...))` we need to save `expr` because it is used in a simple comparison
-            return EvaluateSideEffectingArgumentToTemp(VisitExpression(expr), initEffects, ref temps);
+            BoundExpression loweredExpr = VisitExpression(expr);
+            if ((bool)(loweredExpr.Type != null) && NullableAlwaysHasValue((BoundExpression)loweredExpr) is var value && value != null)


loweredExpr.Type [](start = 23, length = 16)

Cast to object? #Closed

AlekseyTs · 2018-03-22T00:02:18Z

            // in `(..., expr) == (..., (..., ...))` we need to save `expr` because it is used in a simple comparison
-            return EvaluateSideEffectingArgumentToTemp(VisitExpression(expr), initEffects, ref temps);
+            BoundExpression loweredExpr = VisitExpression(expr);
+            if ((bool)(loweredExpr.Type != null) && NullableAlwaysHasValue((BoundExpression)loweredExpr) is var value && value != null)


var [](start = 108, length = 3)

We don't allow usage of var like that. I suggest declaring a local before the if and do (value = NullableAlwaysHasValue((BoundExpression)loweredExpr)) != null instead #Closed

AlekseyTs · 2018-03-22T00:04:30Z

+                    constantValueOpt: null, initializerExpressionOpt: null, binderOpt: null, type: loweredExpr.Type);
+            }
+
+            return EvaluateSideEffectingArgumentToTemp(loweredExpr, initEffects, ref temps);


return EvaluateSideEffectingArgumentToTemp(loweredExpr, initEffects, ref temps); [](start = 12, length = 80)

We should also optimize NullableNeverHasValue case, an expression like that doesn't need caching #Closed

I don't think it needs to be handled specially.
There are two cases:

an element that never has value; then it has no side effects, and will not be saved to a temp anyways, and the lowering for element-wise binary operator will handle it,

a tuple that never has value: this case could be further optimized, but the way of getting that scenario is very contrived and the complexity that adds to the code for rewriting nested case does not seem worthwhile to me. #Closed

an element that never has value; then it has no side effects, and will not be saved to a temp anyways

It is not clear what makes you think this. Have you tried testing with something other than null or default?

a tuple that never has value: this case could be further optimized, but the way of getting that scenario is very contrived and the complexity that adds to the code for rewriting nested case does not seem worthwhile to me.

I don't believe this will add much complexity. Somehow TestEqualOnNullableVsNullableTuples_TupleAlwaysNull screams at me that we should do better. For whatever reason we thought that handling cases like this specially for regular comparisons is worthwhile, I cannot think of any reason why this should be different for tuple comparison. Also, consistency is a good thing.

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

I've tried multiple expressions, such as new and casts. The lowered node for elements that never have a value is a bound default expression, which is recognized have having no side effects.

I agree that the generated IL could be simpler (that's why I included that test).
In terms of complexity, I had tried this over the weekend and found it complex (there were repercussions which I don't remember the details of), especially in the context of this rewriting which is already not simply. I'd rather add a comment to mention the "never null" case.
I can try again, but frankly I feel this is a waste of time, as writing nullable tuples that are always null is contrived (cast or new) in the context of tuple equality.

In reply to: 176505498 [](ancestors = 176505498,176383420)

Ok, I think the problem is that I wanted to skip computing the GetValueOrDefault portion in that case, but I can leave it in (it will be optimized away anyways).

In reply to: 176557640 [](ancestors = 176557640,176505498,176383420)

AlekseyTs · 2018-03-22T00:11:21Z

+                // that temp so it can be recognized as always having a value later on.
+                var savedValue = EvaluateSideEffectingArgumentToTemp(value, initEffects, ref temps);
+                SyntaxNode syntax = loweredExpr.Syntax;
+                _ = TryGetSpecialTypeMethod(syntax, SpecialMember.System_Nullable_T__ctor, out MethodSymbol nullableCtor);


_ = TryGetSpecialTypeMethod(syntax, SpecialMember.System_Nullable_T__ctor, out MethodSymbol nullableCtor); [](start = 15, length = 107)

I think, there is no need to go through all this trouble. Just update the argument in, loweredExpr, it is already an object creation, NullableAlwaysHasValue made sure of that. Especially that we are not supposed to ignore value returned by TryGetSpecialTypeMethod #Closed

AlekseyTs · 2018-03-22T00:14:38Z

+                else
+                {
+                    rightHasValue = MakeBooleanConstant(right.Syntax, true);
+                    rightValue = value;


rightValue = value; [](start = 20, length = 19)

isRightNullable = false; ? #Closed

It is used on line 201 below if (!isLeftNullable && !isRightNullable)

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

Ah, thanks

In reply to: 176501791 [](ancestors = 176501791,176278184)

AlekseyTs · 2018-03-22T00:15:15Z

+                else
+                {
+                    leftHasValue = MakeBooleanConstant(left.Syntax, true);
+                    leftValue = value;


leftValue = value; [](start = 20, length = 18)

isLeftNullable = false; ? #Closed

isLeftNullable isn't used below. #Closed

isLeftNullable isn't used below.

It is used on line 201 below if (!isLeftNullable && !isRightNullable)

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

AlekseyTs · 2018-03-22T00:16:09Z

-                leftHasValue = MakeHasValueTemp(left, temps, outerEffects);
-                leftValue = MakeValueOrDefaultTemp(left, temps, innerEffects);
+                BoundExpression value = NullableAlwaysHasValue(left);
+                if (value is null)


if (value is null) [](start = 16, length = 18)

Should also handle NullableNeverHasValue cases #Closed

AlekseyTs · 2018-03-22T03:05:33Z

+  IL_0001:  ldloca.s   V_3
+  IL_0003:  dup
+  IL_0004:  initobj    ""(int, int)?""
+  IL_000a:  call       ""bool (int, int)?.HasValue.get""


""bool (int, int)?.HasValue.get"" [](start = 23, length = 33)

I'd expect to see much simpler IL. #Closed

Like for the test below

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

AlekseyTs · 2018-03-22T03:07:18Z

Done with review pass (iteration 1) #Closed

gafter · 2018-03-22T16:47:03Z

+                    // with a temp saving that value
+                    BoundExpression savedValue = EvaluateSideEffectingArgumentToTemp(value, initEffects, ref temps);
+                    var objectCreation = (BoundObjectCreationExpression)loweredExpr;
+                    return objectCreation.UpdateArgumentsAndInitializer(ImmutableArray.Create(savedValue), objectCreation.ArgumentRefKindsOpt, objectCreation.InitializerExpressionOpt);


It is not a contract of NullableAlwaysHasValue that it only returns true when the argument is an object creation expression. I expect, for example, that we would expand it to also include certain conversions on top of a creation expression, as is done by the extension method of the same name. #Resolved

Alternately, if that is a contract of NullableAlwaysHasValu, it should be documented so we don't break it in the future.

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

Alternately, if that is a contract of NullableAlwaysHasValu, it should be documented so we don't break it in the future.

I agree with that, let's add a comment in NullableAlwaysHasValue saing that if it starts returning true for more complicated nodes, the logic here would have to be adjusted accordingly.

In reply to: 176493243 [](ancestors = 176493243,176492220)

gafter · 2018-03-22T16:51:44Z

+{
+    static void Main()
+    {
+        System.Console.Write($""{(new int?(Identity(42)), 2) == (new int?(42), 2)} "");


Please also try the same thing with a cast to int? instead of a new. #Resolved

AlekseyTs · 2018-03-22T17:27:18Z

Done with review pass (iteration 2) #Closed

AlekseyTs · 2018-03-23T17:04:03Z

+            verifier.VerifyIL("C.Main", @"{
+  // Code size       13 (0xd)
+  .maxstack  2
+  .locals init (System.ValueTuple<int, int> V_0,


.locals init (System.ValueTuple<int, int> V_0, [](start = 2, length = 46)

None of the locals are used, it would be good to confirm that they are going to be removed for release build. #Closed

They're still there.

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

AlekseyTs · 2018-03-23T17:04:49Z

+                System.ValueTuple<int, int> V_1,
+                (int, int)? V_2)
+  IL_0000:  nop
+  IL_0001:  br.s       IL_0003


br.s IL_0003 [](start = 11, length = 19)

There are some unnecessary jumps, it would be good to confirm that they are going to be removed for release build. #Closed

AlekseyTs · 2018-03-23T17:09:23Z

+{
+    public static void Main()
+    {
+        System.Console.Write(((int, int)?)null == ((int, int)?)null);


((int, int)?)null == ((int, int)?)null [](start = 29, length = 38)

It would be good to cover all combinations: left (might be null, never null, always null) vs. right (never null, always null). And switch sides too. #Closed

AlekseyTs

LGTM (iteration 5) modulo some test suggestions

jcouv · 2018-03-23T19:53:37Z

+}
+";
+
+            CompileAndVerify(source, expectedOutput: "True", options: TestOptions.ReleaseExe).VerifyIL("C.M", @"{


📝 I'll take a stab at optimizing this further. #Resolved

jcouv · 2018-03-23T19:56:34Z

+  IL_0007:  nop
+  IL_0008:  ret
+}
+");


📝 The never-null vs. never-null is covered by TestEqualOnNullableVsNullableTuples_NeverNull.
The maybe-null vs. maybe-null is covered by many tests in this file. #Resolved

gafter

jcouv · 2018-03-23T23:37:13Z

test windows_release_unit32_prtest please

jcouv · 2018-03-23T23:37:21Z

test windows_debug_unit64_prtest please

AlekseyTs

LGTM (iteration 7)

jcouv · 2018-03-26T16:29:42Z

@jaredpar for ask-mode approval for 15.7. Thanks

jcouv · 2018-03-26T16:29:58Z

test windows_debug_unit64_prtest please

jaredpar · 2018-03-26T19:25:45Z

approved.

…features/dataflow * dotnet/dev15.7.x: Fix inference when tuple names differ (dotnet#25738) Avoid crash with 'new ref[]' (dotnet#25505) Skip some PDBUsingTests tests to unblock CI (dotnet#25753) Disable Spanish queue Tuple equality: Optimize cases where nullable always has value (dotnet#25644) Bump prerelease version to beta4 Fix failure to initialize RemoteWorkspace in the out-of-process host remove whitespace Removing package dependency for Microsoft.VisualStudio.Text.UI.Wpf in EditorFeatures Removing package dependency for Microsoft.VisualStudio.Text.UI.Wpf in CSharpEditorFeatures

Optimize cases where nullable always has value

26706cf

jcouv added the Area-Compilers label Mar 21, 2018

jcouv added this to the 15.7 milestone Mar 21, 2018

jcouv self-assigned this Mar 21, 2018

jcouv requested a review from a team as a code owner March 21, 2018 04:50

jcouv changed the title ~~Optimize cases where nullable always has value~~ Tuple equality: Optimize cases where nullable always has value Mar 21, 2018

AlekseyTs reviewed Mar 21, 2018

View reviewed changes

AlekseyTs reviewed Mar 22, 2018

View reviewed changes

Address PR feedback from Aleksey

aece7d4

gafter reviewed Mar 22, 2018

View reviewed changes

Julien Couvreur added 3 commits March 22, 2018 13:23

Address PR feedback from Aleksey and Neal

0272a2c

Also optimize case of always null nullables

23b669a

Factor logic out

65fb469

jcouv force-pushed the never-null branch from 8442f45 to 65fb469 Compare March 22, 2018 21:01

AlekseyTs reviewed Mar 23, 2018

View reviewed changes

AlekseyTs approved these changes Mar 23, 2018

View reviewed changes

Optimize 'default((int, int)).ItemN' and add tests

8b32a37

jcouv commented Mar 23, 2018

View reviewed changes

gafter approved these changes Mar 23, 2018

View reviewed changes

Optimize one more case

f89ffef

AlekseyTs approved these changes Mar 24, 2018

View reviewed changes

jaredpar added the Approved to merge label Mar 26, 2018

jcouv merged commit 1f3c89e into dotnet:dev15.7.x Mar 26, 2018

jcouv deleted the never-null branch March 26, 2018 19:31

Conversation

jcouv commented Mar 21, 2018

Customer scenario

Bugs this fixes

Risk

Performance impact

Is this a regression from a previous update?

Uh oh!

jcouv commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Mar 21, 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!

AlekseyTs Mar 21, 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!

AlekseyTs Mar 22, 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!

AlekseyTs Mar 22, 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!

jcouv Mar 22, 2018 • edited by AlekseyTs 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!

AlekseyTs Mar 22, 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!

AlekseyTs Mar 22, 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!

AlekseyTs Mar 22, 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!

jcouv Mar 22, 2018 • edited by AlekseyTs 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!

AlekseyTs Mar 22, 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!

AlekseyTs Mar 22, 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!

AlekseyTs commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Mar 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Mar 22, 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!

jcouv commented Mar 21, 2018 •

edited

Loading

AlekseyTs Mar 21, 2018 •

edited

Loading

AlekseyTs Mar 21, 2018 •

edited

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

jcouv Mar 22, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

jcouv Mar 22, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

AlekseyTs Mar 22, 2018 •

edited

Loading

AlekseyTs commented Mar 22, 2018 •

edited

Loading

gafter Mar 22, 2018 •

edited by jcouv

Loading

gafter Mar 22, 2018 •

edited

Loading

gafter Mar 22, 2018 •

edited by jcouv

Loading

AlekseyTs commented Mar 22, 2018 •

edited

Loading

AlekseyTs Mar 23, 2018 •

edited

Loading

AlekseyTs Mar 23, 2018 •

edited

Loading

AlekseyTs Mar 23, 2018 •

edited

Loading

jcouv Mar 23, 2018 •

edited

Loading

jcouv Mar 23, 2018 •

edited

Loading