Avoid sharing NullableWalker state between method and nested functions #49765

cston · 2020-12-03T09:18:38Z

When analyzing a method in NullableWalker, avoid sharing _variableTypes and _variableSlots between that method and any lambdas or local functions. Currently, the parameters and locals for any nested functions are added to the _variableTypes and _variableSlots tables in the containing method, adding to the state tracked for containing method and any subsequent nested functions. If there are many nested functions, this can significantly increase the size of the tables that are tracked, particularly for the semantic model.

Fixes #49745

AlekseyTs · 2020-12-03T16:42:55Z

@cston Could you provide more details about the issue? Is this just a matter of "lifetime" for the slots, or did we add new slots for the same things every time we analyzed the same local function on a new pass? #Closed

AlekseyTs · 2020-12-03T17:01:36Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -2623,6 +2623,18 @@ private void VisitStatementsWithLocalFunctions(BoundBlock block)

            var oldState = this.State;
            this.State = state;
+            var oldVariableSlot = _variableSlot;
+            _variableSlot = PooledDictionary<VariableIdentifier, int>.GetInstance();


_variableSlot [](start = 12, length = 13)

Should we also capture and reset variableBySlot and nextVariableSlot? #Closed

Good catch, thanks.

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

cston · 2020-12-03T17:02:29Z

@AlekseyTs, I've updated the description to give details, thanks.

AlekseyTs · 2020-12-03T17:05:30Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            _variableTypes.Free();
+            _variableTypes = oldVariableTypes;
+            _variableSlot.Free();
+            _variableSlot = oldVariableSlot;


_variableSlot [](start = 12, length = 13)

Is it possible that, while analyzing a local function, we allocate a slot for something that is not scoped to the body of the function and, therefore, the slot should be shared with containing method? #Closed

The containing method state is reset after the nested function is visited so any slots added in the method can be ignored.

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

The containing method state is reset after the nested function is visited so any slots added in the method can be ignored.

That may not be true. _localFuncVarUsages may have been updated and that tracks state as well.

In reply to: 535547721 [](ancestors = 535547721,535419178)

AlekseyTs · 2020-12-03T17:48:19Z

Done with review pass (commit 1) #Closed

RikkiGibson · 2020-12-03T22:56:48Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            // with variables in the outer function that were first used in the nested function,
+            // such as a field access on a captured local, but the state associated with
+            // any such entries are dropped, so the slots can be dropped as well.)
+            // We don't drop slots and types if there are local function usages but the


Should this read "...if there are local function usages, because the _localFuncVarUsages dictionary..."? #Resolved

AlekseyTs · 2020-12-03T23:03:56Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                {
+                    _variableTypes.Add(pair.Key, pair.Value);
+                }
+                variableBySlot = new VariableIdentifier[oldVariableBySlot.Length];


variableBySlot = new VariableIdentifier[oldVariableBySlot.Length]; [](start = 16, length = 66)

Consider if we can simply zero out elements in the array starting with oldNextVariableSlot after the analysis.

AlekseyTs

LGTM (commit 5), modulo the CI failure.

AlekseyTs · 2020-12-04T05:34:36Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+
+            public override BoundNode? Visit(BoundNode? node)
+            {
+                if (node is null)


if (node is null) [](start = 16, length = 17)

Consider checking _hasComplexity and returning early when it is already true. #Closed

AlekseyTs · 2020-12-04T05:49:23Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            {
+                if (node is BoundMethodBodyBase methodBody)
+                {
+                    node = methodBody.BlockBody ?? methodBody.ExpressionBody;


node = methodBody.BlockBody ?? methodBody.ExpressionBody; [](start = 20, length = 57)

It feels like we should check both, we are visiting both if present in AbstractFlowPass.VisitMethodBodies.
For BoundConstructorMethodBody we also visit initializer. #Closed

AlekseyTs · 2020-12-04T06:54:27Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -418,6 +424,68 @@ protected override void Free()
            }
        }

+        internal sealed class IsSimpleMethodVisitor : BoundTreeWalkerWithStackGuard


BoundTreeWalkerWithStackGuard [](start = 54, length = 29)

Consider inheriting from BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator to avoid stack overflow on deeply nested binary operators.

AlekseyTs

LGTM (commit 9)

…functions (dotnet#49765)" This reverts commit dedcd58.

cston requested a review from a team as a code owner December 3, 2020 09:18

Dotnet-GitSync-Bot added the Area-Compilers label Dec 3, 2020

cston requested review from RikkiGibson and jaredpar December 3, 2020 09:19

cston added the New Language Feature - Nullable Reference Types Nullable Reference Types label Dec 3, 2020

cston mentioned this pull request Dec 3, 2020

Avoid sharing NullableWalker state between method and nested functions #49766

Closed

cston force-pushed the 49745 branch 2 times, most recently from fd3831c to c96cc14 Compare December 3, 2020 13:31

AlekseyTs reviewed Dec 3, 2020

View reviewed changes

runfoapp bot mentioned this pull request Dec 3, 2020

TestCompleteParenthesisForExtensionMethodImportCompletionProvider is flaky #49399

Closed

RikkiGibson self-assigned this Dec 3, 2020

cston added 3 commits December 3, 2020 13:54

Avoid sharing NullableWalker state between method and nested functions

b034e6b

PR feedback

cedcc66

Check _localFuncVarUsages

9fd6f69

cston force-pushed the 49745 branch from 050e6d9 to 9fd6f69 Compare December 3, 2020 22:01

cston changed the base branch from master to release/dev16.8 December 3, 2020 22:02

cston added 2 commits December 3, 2020 14:05

Misc.

a561ee0

Code cleanup

20f7f6a

RikkiGibson reviewed Dec 3, 2020

View reviewed changes

AlekseyTs reviewed Dec 3, 2020

View reviewed changes

AlekseyTs approved these changes Dec 3, 2020

View reviewed changes

Handle try/finally blocks

6e4492b

RikkiGibson approved these changes Dec 4, 2020

View reviewed changes

Check for specific BoundNodes

76d1733

jaredpar approved these changes Dec 4, 2020

View reviewed changes

AlekseyTs reviewed Dec 4, 2020

View reviewed changes

cston added 2 commits December 3, 2020 22:47

PR feedback

5a29e0d

Remove parameter

fe78b33

AlekseyTs reviewed Dec 4, 2020

View reviewed changes

AlekseyTs approved these changes Dec 4, 2020

View reviewed changes

jinujoseph merged commit dedcd58 into dotnet:release/dev16.8 Dec 4, 2020

cston added this to the 16.8 milestone Dec 4, 2020

This was referenced Dec 4, 2020

Avoid sharing NullableWalker state between containing method and lambdas and local functions #49745

Closed

Runaway memory consumption with build 16.8.0+126527ff1 #49686

Closed

cston added a commit to cston/roslyn that referenced this pull request Dec 5, 2020

Revert "Avoid sharing NullableWalker state between method and nested …

2610141

…functions (dotnet#49765)" This reverts commit dedcd58.

runfoapp bot mentioned this pull request Dec 8, 2020

Roslyn infrastructure failures cause CI to fail #49023

Open

cston added a commit to cston/roslyn that referenced this pull request Jan 15, 2021

Revert "Avoid sharing NullableWalker state between method and nested …

cf5caa3

…functions (dotnet#49765)" This reverts commit dedcd58.

RikkiGibson mentioned this pull request Feb 19, 2021

Seemingly spurious CS8619 when calling ToDictionary after recent VS update #50286

Closed

cston deleted the 49745 branch March 15, 2024 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid sharing NullableWalker state between method and nested functions #49765

Avoid sharing NullableWalker state between method and nested functions #49765

cston commented Dec 3, 2020 •

edited

Loading

AlekseyTs commented Dec 3, 2020 •

edited

Loading

AlekseyTs Dec 3, 2020 •

edited

Loading

cston Dec 3, 2020

cston commented Dec 3, 2020

AlekseyTs Dec 3, 2020 •

edited

Loading

cston Dec 3, 2020

cston Dec 3, 2020

AlekseyTs commented Dec 3, 2020 •

edited

Loading

RikkiGibson Dec 3, 2020 •

edited by cston

Loading

AlekseyTs Dec 3, 2020

AlekseyTs left a comment

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs left a comment

Avoid sharing NullableWalker state between method and nested functions #49765

Avoid sharing NullableWalker state between method and nested functions #49765

Conversation

cston commented Dec 3, 2020 • edited Loading

AlekseyTs commented Dec 3, 2020 • edited Loading

AlekseyTs Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

cston Dec 3, 2020

Choose a reason for hiding this comment

cston commented Dec 3, 2020

AlekseyTs Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

cston Dec 3, 2020

Choose a reason for hiding this comment

cston Dec 3, 2020

Choose a reason for hiding this comment

AlekseyTs commented Dec 3, 2020 • edited Loading

RikkiGibson Dec 3, 2020 • edited by cston Loading

Choose a reason for hiding this comment

AlekseyTs Dec 3, 2020

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Dec 3, 2020 •

edited

Loading

AlekseyTs commented Dec 3, 2020 •

edited

Loading

AlekseyTs Dec 3, 2020 •

edited

Loading

AlekseyTs Dec 3, 2020 •

edited

Loading

AlekseyTs commented Dec 3, 2020 •

edited

Loading

RikkiGibson Dec 3, 2020 •

edited by cston

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading