Skip to content

Commit

Permalink
Remove CapturedVariablesByLambda (#20878)
Browse files Browse the repository at this point in the history 
Swaps out all uses of CapturedVariablesByLambda for functions on the
Scope tree.

The reason the baseline changed is that the order of enumerating closures changed. I briefly looked into matching the behavior, but the problem is that the closure order in the old visitors is determined by the visitation of captured variables, not visitation of the closures themselves. More specifically, an item is only added to the CapturedVariablesByLambda dictionary when ReferenceVariable encounters a captured variable.

In contrast, the visitation of the Scope tree looks at closures in an in-order traversal of the tree, stopping at the first introduced closure, not necessarily the closure which first captured a variable.

One obvious consequence of this changing is that the old ordering for a series of nested lambdas was effectively post-order -- the interior closures would be added to the visitation first, since they would be added from the perspective of the captured variable looking up, instead of the visitor looking down. This particular case is responsible for all the baseline changes that I have seen.

When I looked into replicating this traversal order I found it complicated and pretty counter-intuitive. Since the baseline change was so low in emitted IL, I thought it better to keep a very natural in-order tree traversal and just rebaseline a few tests.

Fixes https://github.com/dotnet/roslyn/projects/26#card-3753318
  • Loading branch information
@agocke
agocke authored Jul 17, 2017
1 parent aeaca17 commit e240a70
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 175 deletions.
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ internal sealed partial class Analysis : BoundTreeWalkerWithStackGuardWithoutRec
/// method being analyzed and has a null <see cref="Parent" />.
/// </summary>
[DebuggerDisplay("{ToString(), nq}")]
private sealed class Scope
public sealed class Scope
{
public Scope Parent { get; }

Expand Down Expand Up @@ -93,7 +93,7 @@ public void Free()
/// variables into captured environments and for calculating
/// the rewritten signature of the method.
/// </summary>
private sealed class Closure
public sealed class Closure
{
/// <summary>
/// The method symbol for the original lambda or local function.
Expand Down Expand Up @@ -124,7 +124,7 @@ private void RemoveUnneededReferences()
var capturesThis = new HashSet<MethodSymbol>();
var capturesVariable = new HashSet<MethodSymbol>();
var visitStack = new Stack<MethodSymbol>();
VisitClosures(_scopeTree, (scope, closure) =>
VisitClosures(ScopeTree, (scope, closure) =>
{
foreach (var capture in closure.CapturedVariables)
{
Expand Down Expand Up @@ -160,7 +160,7 @@ private void RemoveUnneededReferences()
}
}

VisitClosures(_scopeTree, (scope, closure) =>
VisitClosures(ScopeTree, (scope, closure) =>
{
if (!capturesVariable.Contains(closure.OriginalMethodSymbol))
{
Expand All @@ -176,7 +176,7 @@ private void RemoveUnneededReferences()
/// <summary>
/// Visit all closures in all nested scopes and run the <paramref name="action"/>.
/// </summary>
private static void VisitClosures(Scope scope, Action<Scope, Closure> action)
public static void VisitClosures(Scope scope, Action<Scope, Closure> action)
{
foreach (var closure in scope.Closures)
{
Expand Down
165 changes: 61 additions & 104 deletions src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,6 @@ internal sealed partial class Analysis : BoundTreeWalkerWithStackGuardWithoutRec
/// </summary>
public MultiDictionary<Symbol, SyntaxNode> CapturedVariables = new MultiDictionary<Symbol, SyntaxNode>();

/// <summary>
/// For each lambda in the code, the set of variables that it captures.
/// </summary>
public OrderedMultiDictionary<MethodSymbol, Symbol> CapturedVariablesByLambda = new OrderedMultiDictionary<MethodSymbol, Symbol>();

/// <summary>
/// If a local function is in the set, at some point in the code it is converted to a delegate and should then not be optimized to a struct closure.
/// Also contains all lambdas (as they are converted to delegates implicitly).
Expand Down Expand Up @@ -105,7 +100,10 @@ public bool CanTakeRefParameters(MethodSymbol closure) => !(closure.IsAsync
/// </summary>
public Dictionary<MethodSymbol, BoundNode> LambdaScopes;

private Scope _scopeTree;
/// <summary>
/// The root of the scope tree for this method.
/// </summary>
public Scope ScopeTree { get; private set; }

private Analysis(MethodSymbol method)
{
Expand All @@ -123,7 +121,7 @@ public static Analysis Analyze(BoundNode node, MethodSymbol method)

private void Analyze(BoundNode node)
{
_scopeTree = ScopeTreeBuilder.Build(node, this);
ScopeTree = ScopeTreeBuilder.Build(node, this);
_currentScope = FindNodeToAnalyze(node);

Debug.Assert(!_inExpressionLambda);
Expand Down Expand Up @@ -180,85 +178,17 @@ private static BoundNode FindNodeToAnalyze(BoundNode node)
}
}

/// <summary>
/// The old version of <see cref="RemoveUnneededReferences"/> This is still necesary
/// because it modifies <see cref="CapturedVariablesByLambda"/>, which is still used
/// in other areas of the code. Once those uses are gone, this method should be removed.
/// </summary>
private void OldRemoveUnneededReferences()
{
// Note: methodGraph is the inverse of the dependency graph
var methodGraph = new MultiDictionary<MethodSymbol, MethodSymbol>();
var capturesThis = new HashSet<MethodSymbol>();
var capturesVariable = new HashSet<MethodSymbol>();
var visitStack = new Stack<MethodSymbol>();
foreach (var methodKvp in CapturedVariablesByLambda)
{
foreach (var capture in methodKvp.Value)
{
var method = capture as MethodSymbol;
if (method != null)
{
methodGraph.Add(method, methodKvp.Key);
}
else if (capture == _topLevelMethod.ThisParameter)
{
if (capturesThis.Add(methodKvp.Key))
{
visitStack.Push(methodKvp.Key);
}
}
else if (capturesVariable.Add(methodKvp.Key) && !capturesThis.Contains(methodKvp.Key)) // if capturesThis contains methodKvp, it's already in the stack.
{
visitStack.Push(methodKvp.Key);
}
}
}

while (visitStack.Count > 0)
{
var current = visitStack.Pop();
var setToAddTo = capturesVariable.Contains(current) ? capturesVariable : capturesThis;
foreach (var capturesCurrent in methodGraph[current])
{
if (setToAddTo.Add(capturesCurrent))
{
visitStack.Push(capturesCurrent);
}
}
}

var capturedVariablesByLambdaNew = new OrderedMultiDictionary<MethodSymbol, Symbol>();
foreach (var old in CapturedVariablesByLambda)
{
if (capturesVariable.Contains(old.Key))
{
foreach (var oldValue in old.Value)
{
capturedVariablesByLambdaNew.Add(old.Key, oldValue);
}
}
else if (capturesThis.Contains(old.Key))
{
capturedVariablesByLambdaNew.Add(old.Key, _topLevelMethod.ThisParameter);
}
}
CapturedVariablesByLambda = capturedVariablesByLambdaNew;
}

/// <summary>
/// Create the optimized plan for the location of lambda methods and whether scopes need access to parent scopes
/// </summary>
internal void ComputeLambdaScopesAndFrameCaptures()
{
// We need to keep this around
OldRemoveUnneededReferences();
RemoveUnneededReferences();

LambdaScopes = new Dictionary<MethodSymbol, BoundNode>(ReferenceEqualityComparer.Instance);
NeedsParentFrame = new HashSet<BoundNode>();

VisitClosures(_scopeTree, (scope, closure) =>
VisitClosures(ScopeTree, (scope, closure) =>
{
if (closure.CapturedVariables.Count > 0)
{
Expand All @@ -285,7 +215,8 @@ internal void ComputeLambdaScopesAndFrameCaptures()
{
if (captured is LocalFunctionSymbol localFunc)
{
capturedVars.AddAll(FindClosureInScope(closureScope, localFunc).CapturedVariables);
var (found, _) = GetVisibleClosure(closureScope, localFunc);
capturedVars.AddAll(found.CapturedVariables);
}
}

Expand Down Expand Up @@ -320,22 +251,6 @@ internal void ComputeLambdaScopesAndFrameCaptures()
return (innermost, outermost);
}

// Walk up the scope tree looking for a closure
Closure FindClosureInScope(Scope startingScope, MethodSymbol closureSymbol)
{
var currentScope = startingScope;
while (currentScope != null)
{
var found = currentScope.Closures.SingleOrDefault(c => c.OriginalMethodSymbol == closureSymbol);
if (found != null)
{
return found;
}
currentScope = currentScope.Parent;
}
return null;
}

void RecordClosureScope(Scope innermost, Scope outermost, Closure closure)
{
// 1) if there is innermost scope, lambda goes there as we cannot go any higher.
Expand Down Expand Up @@ -373,6 +288,59 @@ void RecordClosureScope(Scope innermost, Scope outermost, Closure closure)
}
}

/// <summary>
/// Walk up the scope tree looking for a closure.
/// </summary>
/// <returns>
/// A tuple of the found <see cref="Closure"/> and the <see cref="Scope"/> it was found in.
/// </returns>
public static (Closure, Scope) GetVisibleClosure(Scope startingScope, MethodSymbol closureSymbol)
{
var currentScope = startingScope;
while (currentScope != null)
{
foreach (var closure in currentScope.Closures)
{
if (closure.OriginalMethodSymbol == closureSymbol)
{
return (closure, currentScope);
}
}
currentScope = currentScope.Parent;
}
throw ExceptionUtilities.Unreachable;
}

/// <summary>
/// Finds a <see cref="Closure"/> with a matching original symbol somewhere in the given scope or nested scopes.
/// </summary>
public static Closure GetClosureInTree(Scope treeRoot, MethodSymbol closureSymbol)
{
return Helper(treeRoot) ?? throw ExceptionUtilities.Unreachable;

Closure Helper(Scope scope)
{
foreach (var closure in scope.Closures)
{
if (closure.OriginalMethodSymbol == closureSymbol)
{
return closure;
}
}

foreach (var nestedScope in scope.NestedScopes)
{
var found = Helper(nestedScope);
if (found != null)
{
return found;
}
}

return null;
}
}

/// <summary>
/// Compute the nesting depth of a given block.
/// Top-most block (where method locals and parameters are defined) are at the depth 0.
Expand Down Expand Up @@ -563,17 +531,6 @@ private void ReferenceVariable(SyntaxNode syntax, Symbol symbol)
if (_currentParent is MethodSymbol lambda && symbol != lambda && symbol.ContainingSymbol != lambda)
{
CapturedVariables.Add(symbol, syntax);

// mark the variable as captured in each enclosing lambda up to the variable's point of declaration.
while ((object)lambda != null &&
symbol != lambda &&
symbol.ContainingSymbol != lambda &&
// Necessary because the EE can insert non-closure synthesized method symbols
IsClosure(lambda))
{
CapturedVariablesByLambda.Add(lambda, symbol);
lambda = lambda.ContainingSymbol as MethodSymbol;
}
}
}

Expand Down
47 changes: 22 additions & 25 deletions src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,22 +328,20 @@ protected override NamedTypeSymbol ContainingType
/// </summary>
private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
{
var closures = _analysis.CapturedVariablesByLambda.Keys;

foreach (var closure in closures)
Analysis.VisitClosures(_analysis.ScopeTree, (scope, closure) =>
{
var capturedVars = _analysis.CapturedVariablesByLambda[closure];
bool canTakeRefParams = _analysis.CanTakeRefParameters(closure);
var capturedVars = closure.CapturedVariables;
MethodSymbol closureSymbol = closure.OriginalMethodSymbol;
bool canTakeRefParams = _analysis.CanTakeRefParameters(closureSymbol);

if (canTakeRefParams && OnlyCapturesThis((LocalFunctionSymbol)closure, capturedVars))
if (canTakeRefParams && OnlyCapturesThis(closure, scope))
{
continue;
return;
}

foreach (var captured in capturedVars)
{
BoundNode scope;
if (!_analysis.VariableScope.TryGetValue(captured, out scope))
if (!_analysis.VariableScope.TryGetValue(captured, out BoundNode captureScope))
{
continue;
}
Expand All @@ -359,7 +357,7 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
continue;
}

LambdaFrame frame = GetFrameForScope(scope, closureDebugInfo);
LambdaFrame frame = GetFrameForScope(captureScope, closureDebugInfo);

if (captured.Kind != SymbolKind.Method && !proxies.ContainsKey(captured))
{
Expand All @@ -377,30 +375,31 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
}
}
}
}
});
}


private SmallDictionary<LocalFunctionSymbol, bool> _onlyCapturesThisMemoTable;
private SmallDictionary<Analysis.Closure, bool> _onlyCapturesThisMemoTable;
/// <summary>
/// Helper for determining whether a local function transitively
/// only captures this (only captures this or other local functions
/// which only capture this).
/// </summary>
private bool OnlyCapturesThis<T>(
LocalFunctionSymbol closure,
T capturedVars,
private bool OnlyCapturesThis(
Analysis.Closure closure,
Analysis.Scope scope,
PooledHashSet<LocalFunctionSymbol> localFuncsInProgress = null)
where T : IEnumerable<Symbol>
{
Debug.Assert(closure != null);
Debug.Assert(scope != null);

bool result = false;
if (_onlyCapturesThisMemoTable?.TryGetValue(closure, out result) == true)
{
return result;
}

result = true;
foreach (var captured in capturedVars)
foreach (var captured in closure.CapturedVariables)
{
var param = captured as ParameterSymbol;
if (param != null && param.IsThis)
Expand All @@ -423,10 +422,8 @@ private bool OnlyCapturesThis<T>(
}

localFuncsInProgress.Add(localFunc);
bool transitivelyTrue = OnlyCapturesThis(
localFunc,
_analysis.CapturedVariablesByLambda[localFunc],
localFuncsInProgress);
var (found, foundScope) = Analysis.GetVisibleClosure(scope, localFunc);
bool transitivelyTrue = OnlyCapturesThis(found, foundScope, localFuncsInProgress);

if (freePool)
{
Expand All @@ -446,7 +443,7 @@ private bool OnlyCapturesThis<T>(

if (_onlyCapturesThisMemoTable == null)
{
_onlyCapturesThisMemoTable = new SmallDictionary<LocalFunctionSymbol, bool>();
_onlyCapturesThisMemoTable = new SmallDictionary<Analysis.Closure, bool>();
}

_onlyCapturesThisMemoTable[closure] = result;
Expand Down Expand Up @@ -474,7 +471,7 @@ private LambdaFrame GetFrameForScope(BoundNode scope, ArrayBuilder<ClosureDebugI
frame = new LambdaFrame(_topLevelMethod, containingMethod, canBeStruct, syntax, methodId, closureId);
_frames.Add(scope, frame);

CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(this.ContainingType, frame);
CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(ContainingType, frame);
if (frame.Constructor != null)
{
AddSynthesizedMethod(
Expand Down Expand Up @@ -1335,7 +1332,7 @@ private SynthesizedLambdaMethod RewriteLambdaOrLocalFunction(
closureOrdinal = containerAsFrame.ClosureOrdinal;
}
}
else if (_analysis.CapturedVariablesByLambda[node.Symbol].Count == 0)
else if (Analysis.GetClosureInTree(_analysis.ScopeTree, node.Symbol).CapturedVariables.Count == 0)
{
if (_analysis.MethodsConvertedToDelegates.Contains(node.Symbol))
{
Expand Down
Loading

0 comments on commit e240a70

Please sign in to comment.