Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove CapturedVariablesByLambda #20878

Merged
merged 3 commits into from
Jul 17, 2017
Merged

Remove CapturedVariablesByLambda #20878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6064aac
Remove CapturedVariablesByLambda
agocke Jul 12, 2017
4da70bc
Re-baseline IL in EE test
agocke Jul 14, 2017
21924dd
PR comments
agocke Jul 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs
View file
Open in desktop
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
168 changes: 64 additions & 104 deletions src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs
View file
Open in desktop
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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the private set. It's unnecessary since you only modify the value in the ctor. Makes it clear this is a readonly property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? It's set in the visitor, not in the constructor. I could set it in the constructor but that's a dangerous dance as I would be handing out this before fully constructed.


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, _) = FindClosureInScope(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,62 @@ 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) FindClosureInScope(Scope startingScope, MethodSymbol closureSymbol)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider GetClosureInScope vs. Find. The general rule for funtions like this is:

  • Find: returns null on failure. Essentially a method that expects failure as an outcome.
  • Get: throws on failure. A method which expects to find the value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm also consider simply GetClosure. The InScope part to me can read as "find a closure inside this scope" which is contrary to the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to find some way to say "this method searches up", as opposed to the other one, which searches down. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetVisibleClosure?

{
var currentScope = startingScope;
while (currentScope != null)
{
var found = currentScope.Closures.SingleOrDefault(c => c.OriginalMethodSymbol == closureSymbol);
if (found != null)
{
return (found, 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 FindClosureInTree(Scope rootScope, MethodSymbol closureSymbol)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another place to consider Get vs. Find

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using treeRoot vs. rootScope. The term root scope has a particular meaning in this code base already: the absolute root of the scopes. This method though works on any arbitrary scope.

{
var result = Helper(rootScope);
if (result != null)
{
return result;
}
throw ExceptionUtilities.Unreachable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return result ?? throw ...

Don't actually care if you do this, just a reminder to myself that you could 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

niiice


Closure Helper(Scope scope)
{
foreach (var closure in scope.Closures)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the FindClosureInScope you used SingleOrDefault to accomplish the exact same operation. Why use foreach here but SingleOrDefault there? Seems like these should do the same operation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change the other one to use a foreach loop too. Avoids allocating a capture environment and a delegate 😄

{
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 +534,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
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this why the names changed? We are traversing closures in different order? I think this is ok.
I think we may be changing the order more in the future. It may make sense eventually for the frame generator to just traverse Scopes/DeclaredVariables instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

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.FindClosureInScope(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.FindClosureInTree(_analysis.ScopeTree, node.Symbol).CapturedVariables.Count == 0)
{
if (_analysis.MethodsConvertedToDelegates.Contains(node.Symbol))
{
Expand Down
Loading