Introduce local caching of "this" in async methods #14959

VSadov · 2016-11-04T02:37:27Z

Introduce local caching of "this" in async methods

JIT has no knowledge that captured "this" is semantically readonly, so it would fetch the captured "this" repeatedly if instance members are accessed in the method (which is fairly common). Compiler has such knowledge and can help JIT by caching the reference into a local.

Also, we do not need to call nonvirtual methods off captured "this" via calvirt. For the same reasons we do not use calvirt when calling through the actual "this" directly.

Calvirt here has extra costs. It forces the JIT to insert IR for null checks and later JIT tries to proof that these checks might be unnecessary, which it often can't, since again, it does not have that knowledge that we have at compile time.

Related to: https://github.com/dotnet/coreclr/issues/7914

Fixes:#14873

VSadov · 2016-11-04T02:42:50Z

@dotnet/roslyn-compiler - please review

VSadov · 2016-11-11T00:34:33Z

@dotnet/roslyn-compiler - ping. Anyone can review this?

VSadov · 2016-11-11T16:18:38Z

@dotnet/roslyn-compiler - ping. Anyone can review this?

jaredpar · 2016-11-11T16:44:46Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

-                EmitExpression(fieldAccess.ReceiverOpt, used: false);
-                return;
+                // fetching unused captured frame is a no-op (like reading "this")
+                if(field.IsCapturedFrame)


Missing space between if and ( #Resolved

jaredpar · 2016-11-11T16:45:16Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+                    return;
+                }
+
+                //TODO: For static field access this may require ..ctor to run. Is this a side-effect?


I realize this is an old comment but we should take the time to correct. If this is a real bug please file an issue for the TODO and reference it here. Otherwise if it's not a bug let's clean up this comment. #Resolved

Old comment. accessing static fields is sideeffecting when we have .cctor. The code is not actually trying to be smart about this.
Will clean up comments around this code.

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

Note that generally we cannot generally know whether we have .cctor or not, since the type maybe in a referenced assembly.
In some cases we might know, but does not seem to be worth special casing.

In reply to: 87655591 [](ancestors = 87655591,87618871)

jaredpar · 2016-11-11T16:45:54Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+
+                //TODO: For static field access this may require ..ctor to run. Is this a side-effect?
+                // Accessing unused instance field on a struct is a noop. Just emit the receiver.
+                if (!field.IsVolatile && !field.IsStatic && fieldAccess.ReceiverOpt.Type.IsVerifierValue())


Why are we confident that ReceiverOpt will be non-null here? #ByDesign

I guess, non-static field access should never have a null receiver. #Resolved

Right. Non-static accesses always have an instance.

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

jaredpar · 2016-11-11T16:47:08Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

-                    //if that has already happen, we will just propagate the behavior.
+                    //however, we assume it is responsibility of the caller to nullcheck "this"
+                    //if we already have access to "this", we must be in a member and should 
+                    //not redo the check


Nit: space between comment and word. #Resolved

jaredpar · 2016-11-11T16:50:21Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodToStateMachineRewriter.cs

@@ -425,7 +431,7 @@ private BoundBlock GenerateAwaitForIncompleteTask(LocalSymbol awaiterTemp)
                    // this.state = cachedState = NotStartedStateMachine
                    F.Assignment(F.Field(F.This(), stateField), F.AssignmentExpression(F.Local(cachedState), F.Literal(StateMachineStates.NotStartedStateMachine))));

-            return F.Block(blockBuilder.ToImmutableAndFree());
+             return F.Block(blockBuilder.ToImmutableAndFree());


Nit: extra space? #Resolved

jaredpar · 2016-11-11T16:53:00Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs

@@ -44,6 +44,8 @@ internal abstract class MethodToStateMachineRewriter : MethodToClassRewriter
        /// </summary>
        protected readonly LocalSymbol cachedState;

+        protected readonly LocalSymbol cachedThis;


Can we add a comment to the purpose of this local. In particular the cases where it will be non-null? #Resolved

jaredpar · 2016-11-11T16:53:36Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs

        {
+            // restore "this" cache, if there is a cache
+            if (this.cachedThis != null)


((object)cachedThis) != null ``` #Resolved

jaredpar · 2016-11-11T16:53:49Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs

@@ -768,6 +802,13 @@ public override BoundNode VisitThisReference(BoundThisReference node)
        public override BoundNode VisitBaseReference(BoundBaseReference node)
        {
            // TODO: fix up the type of the resulting node to be the base type
+
+            // if "this" is cached, return it.
+            if (this.cachedThis != null)


Same feedback. #Resolved

jcouv · 2016-11-11T18:31:32Z

        // switch(cachedState) {

Update comment with optional cached this? #Resolved

Refers to: src/Compilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorMethodToStateMachineRewriter.cs:89 in 87831b6. [](commit_id = 87831b671faf977b1c3c8b34c8bb88afc9a090b0, deletion_comment = False)

jcouv · 2016-11-11T18:33:32Z

src/Compilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorMethodToStateMachineRewriter.cs

@@ -95,11 +95,13 @@ internal void GenerateMoveNextAndDispose(BoundStatement body, SynthesizedImpleme
            // state_0:
            // state = -1;
            // [[rewritten body]]
-            newBody = F.Block(ImmutableArray.Create(cachedState),
-                    F.Block(


We didn't need this nested block? #ByDesign

Inner block declares no locals, so it is definitely unnecessary.
Blocks without locals are rarely useful - mostly just for constructs that require blocks (like Try).

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

jcouv · 2016-11-11T18:36:37Z

src/Compilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorMethodToStateMachineRewriter.cs

@@ -386,8 +388,11 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
            //      return;
            // }
            Debug.Assert(frame.parent.finalizeState == _currentFinallyFrame.finalizeState);
-            rewrittenHandler = F.Block(
+            rewrittenHandler = F.Block((object)this.cachedThis != null?
+                                            ImmutableArray.Create(this.cachedThis):


Maybe we should add an extension on ImmutableArray for this common pattern. #WontFix

The extension would be on T: class . It feels like it would be very noisy.

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

jcouv · 2016-11-11T18:44:57Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs

+            if ((object)thisParameter != null && 
+                thisParameter.Type.IsReferenceType &&
+                proxies.TryGetValue(thisParameter, out thisProxy) &&
+                F.Compilation.Options.OptimizationLevel == OptimizationLevel.Release)


Can you verify we have a test with a struct (but we compile in release mode)? #WontFix

We have existing tests which fail if the optimization does not check for a class.

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

jcouv · 2016-11-11T18:48:54Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineFieldSymbol.cs

@@ -61,5 +63,10 @@ IMethodSymbol ISynthesizedMethodBodyImplementationSymbol.Method
        {
            get { return ((ISynthesizedMethodBodyImplementationSymbol)ContainingSymbol).Method; }
        }
+
+        internal override bool IsCapturedFrame


Could you take a look at IsCapturedFrame in FieldSymbol.cs? Do you think the xml-doc needs to be updated? #WontFix

I think the xml-doc looks fine and still applies.

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

jcouv

LGTM

AlekseyTs · 2016-11-11T18:56:27Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs

+                proxies.TryGetValue(thisParameter, out thisProxy) &&
+                F.Compilation.Options.OptimizationLevel == OptimizationLevel.Release)
+            {
+                var thisProxyReplacement = thisProxy.Replacement(F.Syntax, frameType => F.This());


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

Consider spelling out the type. #Resolved

AlekseyTs · 2016-11-11T19:01:12Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs

+            }
+
+            // do nothing
+            return F.Block();


return F.Block(); [](start = 12, length = 17)

Could we return an empty statement list instead? Might be cheaper in terms of memory. #Resolved

AlekseyTs · 2016-11-11T19:08:22Z

LGTM, but please address Jared's feedback about missing conversions to object.

VSadov · 2016-11-12T04:53:03Z

test this please

VSadov · 2016-11-12T16:46:46Z

test windows_debug_unit32_prtest please

VSadov · 2016-11-15T05:11:43Z

CC: @AndyAyersMS

dnfclas added the cla-already-signed label Nov 4, 2016

VSadov added the Area-Compilers label Nov 7, 2016

jaredpar approved these changes Nov 11, 2016

View reviewed changes

jcouv reviewed Nov 11, 2016

View reviewed changes

jcouv approved these changes Nov 11, 2016

View reviewed changes

AlekseyTs reviewed Nov 11, 2016

View reviewed changes

jaredpar approved these changes Nov 11, 2016

View reviewed changes

VSadov added 2 commits November 11, 2016 13:33

Introduce local caching of "this" in async methods

e0e4293

CR feedback

8e5bf07

VSadov force-pushed the cacheThis branch from 9cc20a7 to 8e5bf07 Compare November 11, 2016 21:34

VSadov merged commit bf8d2ec into dotnet:master Nov 12, 2016

CyrusNajmabadi mentioned this pull request Dec 4, 2016

Proposal: "while" as a LINQ query expression clause #15638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce local caching of "this" in async methods #14959

Introduce local caching of "this" in async methods #14959

VSadov commented Nov 4, 2016 •

edited

Loading

VSadov commented Nov 4, 2016

VSadov commented Nov 11, 2016

VSadov commented Nov 11, 2016

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

VSadov Nov 11, 2016

VSadov Nov 11, 2016

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

AlekseyTs Nov 11, 2016 •

edited by VSadov

Loading

VSadov Nov 11, 2016

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jcouv commented Nov 11, 2016 •

edited by VSadov

Loading

jcouv Nov 11, 2016 •

edited by VSadov

Loading

VSadov Nov 11, 2016

jcouv Nov 11, 2016 •

edited by VSadov

Loading

VSadov Nov 11, 2016

jcouv Nov 11, 2016 •

edited by VSadov

Loading

VSadov Nov 11, 2016

jcouv Nov 11, 2016 •

edited by VSadov

Loading

VSadov Nov 11, 2016

jcouv left a comment

AlekseyTs Nov 11, 2016 •

edited by VSadov

Loading

AlekseyTs Nov 11, 2016 •

edited by VSadov

Loading

AlekseyTs commented Nov 11, 2016

VSadov commented Nov 12, 2016

VSadov commented Nov 12, 2016

VSadov commented Nov 15, 2016

Introduce local caching of "this" in async methods #14959

Introduce local caching of "this" in async methods #14959

Conversation

VSadov commented Nov 4, 2016 • edited Loading

VSadov commented Nov 4, 2016

VSadov commented Nov 11, 2016

VSadov commented Nov 11, 2016

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

jaredpar Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

jcouv commented Nov 11, 2016 • edited by VSadov Loading

jcouv Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

jcouv Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

jcouv Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

jcouv Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Nov 11, 2016

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2016 • edited by VSadov Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 11, 2016

VSadov commented Nov 12, 2016

VSadov commented Nov 12, 2016

VSadov commented Nov 15, 2016

VSadov commented Nov 4, 2016 •

edited

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

AlekseyTs Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jaredpar Nov 11, 2016 •

edited by VSadov

Loading

jcouv commented Nov 11, 2016 •

edited by VSadov

Loading

jcouv Nov 11, 2016 •

edited by VSadov

Loading

jcouv Nov 11, 2016 •

edited by VSadov

Loading

jcouv Nov 11, 2016 •

edited by VSadov

Loading

jcouv Nov 11, 2016 •

edited by VSadov

Loading

AlekseyTs Nov 11, 2016 •

edited by VSadov

Loading

AlekseyTs Nov 11, 2016 •

edited by VSadov

Loading