Remove RemoveUnneededReferences from LamdaRewriter #21367

agocke · 2017-08-08T07:09:46Z

Currently, the lambda rewriter has an early optimization pass in
analysis that tries to find all local functions that only capture 'this'
and remove references to local functions that do the same. There are two
problems with this approach:

1) Generally, removing information from the tree is a bad idea
because it hurts further analysis passes that may have needed that
information.

2) The optimization strategy itself is very tricky and has a number
of complex corner cases. This has lead to bugs, for example #19033.

This PR deletes the current method and adds a new optimization routine
at the end of the analysis, operating on assigned scopes and
environments rather than removing captured variable analysis. The new
optimization is as follows: if we end up with an environment containing
only 'this', the environment can be removed, all containing methods can
be moved to the top-level type, and all environments which capture the
'this' environment can instead directly capture the 'this' parameter.
This produces almost the same results as the previous optimization, but
is easier to validate as an algebraic equivalence.

Fixes #19033
Fixes #20577

Currently, the lambda rewriter has an early optimization pass in analysis that tries to find all local functions that only capture 'this' and remove references to local functions that do the same. There are two problems with this approach: 1) Generally, removing information from the tree is a bad idea because it hurts further analysis passes that may have needed that information. 2) The optimization strategy itself is very tricky and has a number of complex corner cases. This has lead to bugs, for example dotnet#19033. This PR deletes the current method and adds a new optimization routine at the end of the analysis, operating on assigned scopes and environments rather than removing captured variable analysis. The new optimization is as follows: if we end up with an environment containing only 'this', the environment can be removed, all containing methods can be moved to the top-level type, and all environments which capture the 'this' environment can instead directly capture the 'this' parameter. This produces almost the same results as the previous optimization, but is easier to validate as an algebraic equivalence. Fixes dotnet#19033, dotnet#20577

The changes come from the new optimization being less aggressive about moving functions which only capture 'this' to the top level. This appears to be a wash -- some codegen gets slightly better, some gets slightly worse.

khyperia

LGTM - the two comments I have are just wild guesses, no idea if my conclusion is right or not.

khyperia · 2017-08-09T21:54:07Z

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

-  IL_0023:  ldfld      ""int C._x""
-  IL_0028:  call       ""void System.Console.WriteLine(int)""
-  IL_002d:  ret
+  IL_001b:  ldarg.0


It looks like we're storing this in both C.<>c__DisplayClass2_0.<>4__this as well as the receiver on C.<M>g__L12_0(ref C.<>c__DisplayClass2_0). Is that intended?

(It looks like C.<M>g__L12_0(ref C.<>c__DisplayClass2_0) never touches the captured C C.<>c__DisplayClass2_1.<>4__this)

Yup, this is a result of a less aggressive optimization. What's going on here is we only eliminate the this closure if it captures only this. But both x and this are captured in the same scope, so the environment contains both x and this, thus the optimization doesn't apply. I haven't found a really simple optimization strategy that works for this case, so I didn't do anything extra here.

If you have any suggestions I could incorporate them, though.

Wait... but it's also lowered onto the containing class. I'm not sure why that is - it seems like if we don't optimize the this out of the environment, the generated method shouldn't go on the containing class. (And grab the field, instead of the this parameter of the generated method, if it needs the original this)

Oh, I think I get it - it is by design. We use the generated this parameter when we can, but the environment may be passed somewhere that we can't use the generated this parameter (such as a lambda placed on the environment class).

Right, and finding and ruling out all those corner cases is the tricky bit, so I haven't built up a full pass that handles everything.

khyperia · 2017-08-09T22:00:08Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/LocalFunctionTests.cs

@@ -123,7 +123,7 @@ int G()
  // Code size        7 (0x7)
  .maxstack  1
  .locals init (int V_0)
-  IL_0000:  ldarg.0
+  IL_0000:  ldarg.1


I'm guessing the same thing happened here - capturing this on both the receiver and a field.

jaredpar · 2017-08-09T22:07:41Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs

+                /// a local function which directly or indirectly captures 'this').
+                /// Calculated in <see cref="MakeAndAssignEnvironments"/>.
+                /// </summary>
+                public bool CapturesThis;


Why fields instead of properties?

Is there ever a case where this can move from true to false? If not would it make sense to validate this in a prooperty setter?

Sounds good. I can do that.

In general though why are you preferring fields here over auto-properties? That seems to go against the style of the compiler.

The original LambdaRewriter was all fields. I was just sticking with the surrounding code style.

jaredpar · 2017-08-09T22:11:06Z

src/Compilers/Core/Portable/InternalUtilities/SetWithInsertionOrder.cs

+                {
+                    return false;
+                }
+                _elements.Insert(index, value);


Consider the case the Insert operation throws because it's invalid. That means SetWithInsertionOrder is now in an inconsistent state: _set has the value but it's not present in _elements. Think we need to guard against this and ensure the data type is consistent in the face of bad index values.

jaredpar · 2017-08-09T22:11:45Z

src/Compilers/Core/Portable/InternalUtilities/SetWithInsertionOrder.cs

@@ -57,5 +78,7 @@ public IEnumerator<T> GetEnumerator()
        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

        public ImmutableArray<T> AsImmutable() => _elements.ToImmutableArrayOrEmpty();
+
+        public T this[int i] => _elements[i];


Should this be a ref return? I'd think so if T is ever a struct

ArrayBuilder doesn't (and can't) have a ref returning indexer, so this can't.

jaredpar · 2017-08-09T23:39:41Z

src/Compilers/Core/Portable/InternalUtilities/SetWithInsertionOrder.cs

+                if (!_set.Add(value))
+                {
+                    return false;
+                }


Blank line.

jaredpar · 2017-08-09T23:40:35Z

src/Compilers/Core/Portable/InternalUtilities/SetWithInsertionOrder.cs

+                }
+                catch
+                {
+                    _set.Remove(value);


Wanted to give feedback here about writing tests but it seems like we don't have any tests for SetWithInsertionOrder.

jaredpar · 2017-08-09T23:40:58Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs

@@ -119,6 +119,22 @@ public sealed class Closure

                public ClosureEnvironment ContainingEnvironmentOpt;

+                private bool _capturesThis;


jaredpar · 2017-08-09T23:42:11Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs

+            /// Visit all the closures and return true when the <paramref name="func"/> returns
+            /// true. Otherwise, returns false.
+            /// </summary>
+            public static bool CheckClosures(Scope scope, Func<Scope, Closure, bool> func)


Is this method used? Can't find a reference in this PR.

In InlineThisOnlyEnvironment when we see if any ContainingEnvironmentOpt is non-null.

jaredpar · 2017-08-09T23:42:57Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs

+                /// variable. Assigned in
+                /// <see cref="ComputeLambdaScopesAndFrameCaptures(ParameterSymbol)"/>
+                /// </summary>
+                public bool CapturesParent;


Can this value go to false after going to true? If so does it make sense to assert?

agocke · 2017-08-10T22:54:28Z

As @khyperia pointed out, there are some situations with this optimization scheme where an instance method may receive a struct environment that also contains a pointer to this. For example, the generated code may look like

class C
{
    void CallMe() {}

    struct S
    {
        C _c;
        int _x;
    }

    int Lowered(ref S s)
    {
        CallMe();
    }
}

You'll notice that in the Lowered method, there are two paths to CallMe -- one through S (s._c.CallMe()) and one through the parent class (this.CallMe()). In these situations, the lowering always prefers the implicit receiver, if possible.

I've done a quick benchmark of the two options, and here are the results:

// * Summary *

BenchmarkDotNet=v0.10.9, OS=Windows 10 Redstone 2 (10.0.15063)
Processor=Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), ProcessorCount=8
Frequency=3515628 Hz, Resolution=284.4442 ns, Timer=TSC
.NET Core SDK=1.1.0
  [Host]     : .NET Core 1.1.2 (Framework 4.6.25211.01), 64bit RyuJIT DEBUG
  DefaultJob : .NET Core 1.1.2 (Framework 4.6.25211.01), 64bit RyuJIT


       Method |      Mean |     Error |    StdDev |
------------- |----------:|----------:|----------:|
 ReceiverCall | 0.0010 ns | 0.0023 ns | 0.0021 ns |
    FieldCall | 0.0215 ns | 0.0242 ns | 0.0215 ns |

As you can see, this approach appears to be about 10x faster than going through the field.

…throw-statement-expression * dotnet/features/ioperation: (41 commits) Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Recommend 'case' keyword after a pattern-case-clause. Fix NamedArgumentInParameterOrderWithDefaultValue test for new IOperation output. Resolving merge conflict ...

…-literal-text * dotnet/features/ioperation: Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Resolving merge conflict Don't pick a project arbitrarily when navigating to symbols

…tatement-refactor * dotnet/features/ioperation: Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Resolving merge conflict Don't pick a project arbitrarily when navigating to symbols

…oalescingexpression-refactor * dotnet/features/ioperation: Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Resolving merge conflict Don't pick a project arbitrarily when navigating to symbols

dnfclas added the cla-already-signed label Aug 8, 2017

agocke added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 8, 2017

agocke force-pushed the remove-removeunneededreferences branch from cf7b209 to a50a481 Compare August 8, 2017 23:14

Update baselines with the new optimization

1de4f54

The changes come from the new optimization being less aggressive about moving functions which only capture 'this' to the top level. This appears to be a wash -- some codegen gets slightly better, some gets slightly worse.

agocke force-pushed the remove-removeunneededreferences branch from a50a481 to 1de4f54 Compare August 8, 2017 23:57

agocke requested review from khyperia, jaredpar, VSadov and a team August 9, 2017 01:24

agocke added Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Aug 9, 2017

agocke mentioned this pull request Aug 9, 2017

Remove LocalFunctionRewriting pass #21408

Merged

khyperia approved these changes Aug 9, 2017

View reviewed changes

jaredpar requested changes Aug 9, 2017

View reviewed changes

PR comments

d360dd2

jaredpar reviewed Aug 9, 2017

View reviewed changes

jaredpar approved these changes Aug 9, 2017

View reviewed changes

agocke added 2 commits August 9, 2017 16:52

More PR comments

0c3a8c1

Remove overly aggressive assert

884a081

agocke merged commit caa7830 into dotnet:master Aug 11, 2017

agocke deleted the remove-removeunneededreferences branch August 11, 2017 03:02

svick mentioned this pull request Aug 23, 2017

Nested local functions causes KeyNotFoundException #21645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove RemoveUnneededReferences from LamdaRewriter #21367

Remove RemoveUnneededReferences from LamdaRewriter #21367

agocke commented Aug 8, 2017 •

edited

khyperia left a comment

khyperia Aug 9, 2017

agocke Aug 9, 2017

khyperia Aug 9, 2017

khyperia Aug 9, 2017 •

edited

agocke Aug 9, 2017 •

edited

khyperia Aug 9, 2017

jaredpar Aug 9, 2017

agocke Aug 9, 2017

jaredpar Aug 9, 2017

agocke Aug 9, 2017

jaredpar Aug 9, 2017

jaredpar Aug 9, 2017

jaredpar Aug 9, 2017

agocke Aug 9, 2017 •

edited

jaredpar Aug 9, 2017

jaredpar Aug 9, 2017

agocke Aug 9, 2017

jaredpar Aug 9, 2017

jaredpar Aug 9, 2017

agocke Aug 9, 2017

jaredpar Aug 9, 2017

agocke commented Aug 10, 2017

		@@ -119,6 +119,22 @@ public sealed class Closure

		public ClosureEnvironment ContainingEnvironmentOpt;

		private bool _capturesThis;

Remove RemoveUnneededReferences from LamdaRewriter #21367

Remove RemoveUnneededReferences from LamdaRewriter #21367

Conversation

agocke commented Aug 8, 2017 • edited

khyperia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khyperia Aug 9, 2017 • edited

Choose a reason for hiding this comment

agocke Aug 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke Aug 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Aug 10, 2017

agocke commented Aug 8, 2017 •

edited

khyperia Aug 9, 2017 •

edited

agocke Aug 9, 2017 •

edited

agocke Aug 9, 2017 •

edited