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.

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

Track state separately in NullableWalker for nested and containing methods #50417

Merged
merged 16 commits into from
Jan 23, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private List<Symbol> Analyze(ref bool badRegion)
{
foreach (var i in _endOfRegionState.Assigned.TrueBits())
{
if (i >= variableBySlot.Length)
if (i >= variableBySlot.Count)
{
continue;
}
Expand Down
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass<
/// The first slot, slot 0, is reserved for indicating reachability, so the first tracked variable will
/// be given slot 1. When referring to VariableIdentifier.ContainingSlot, slot 0 indicates
/// that the variable in VariableIdentifier.Symbol is a root, i.e. not nested within another
/// tracked variable. Slots &lt; 0 are illegal.
/// tracked variable. Slots less than 0 are illegal.
/// </summary>
protected VariableIdentifier[] variableBySlot = new VariableIdentifier[1];
protected readonly ArrayBuilder<VariableIdentifier> variableBySlot = ArrayBuilder<VariableIdentifier>.GetInstance(1, default);

/// <summary>
/// Variable slots are allocated to local variables sequentially and never reused. This is
Expand Down Expand Up @@ -216,6 +216,7 @@ internal DefiniteAssignmentPass(

protected override void Free()
{
variableBySlot.Free();
_variableSlot.Free();
_usedVariables.Free();
_readParameters?.Free();
Expand All @@ -238,11 +239,10 @@ protected override int AddVariable(VariableIdentifier identifier)
{
int slot = nextVariableSlot++;
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 21, 2021

Choose a reason for hiding this comment

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

nextVariableSlot [](start = 23, length = 16)

Would it make sense to get rid of this field altogether? Can we simply look at _variableSlot to figure out the next available slot? #Closed

_variableSlot.Add(identifier, slot);
if (slot >= variableBySlot.Length)
while (slot >= variableBySlot.Count)
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 21, 2021

Choose a reason for hiding this comment

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

while (slot >= variableBySlot.Count) [](start = 12, length = 36)

Is there ever a case when we are not doing exactly one iteration? #Closed

{
Array.Resize(ref this.variableBySlot, slot * 2);
variableBySlot.Add(default);
Copy link
Member

@333fred 333fred Jan 19, 2021

Choose a reason for hiding this comment

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

Consider using AddMany. #Closed

Copy link
Member Author

@cston cston Jan 20, 2021

Choose a reason for hiding this comment

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

slot should be at most equal to, not greater than, variableBySlot.Count so the while loop is executed at most once. The alternative seems a little more complicated:

if (slot >= variableBySlot.Count)
{
    variableBySlot.AddMany(default, variableBySlot.Count - slot + 1);
}

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

}

variableBySlot[slot] = identifier;
return slot;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ private void ProcessState(HashSet<Symbol> definitelyAssigned, LocalState state1,
{
foreach (var slot in state1.Assigned.TrueBits())
{
if (slot < variableBySlot.Length &&
if (slot < variableBySlot.Count &&
state2opt?.IsAssigned(slot) != false &&
variableBySlot[slot].Symbol is { } symbol &&
symbol.Kind != SymbolKind.Field)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,14 @@ private sealed class NextId
private readonly PooledDictionary<Symbol, TypeWithAnnotations> _variableTypes = SpecializedSymbolCollections.GetPooledSymbolDictionaryInstance<Symbol, TypeWithAnnotations>();

/// <summary>
/// A mapping from the local variable slot to the symbol for the local variable itself. This
/// is used in the implementation of region analysis (support for extract method) to compute
/// the set of variables "always assigned" in a region of code.
/// A mapping from the local variable slot to the symbol for the local variable itself.
///
/// The first slot, slot 0, is reserved for indicating reachability, so the first tracked variable will
/// be given slot 1. When referring to VariableIdentifier.ContainingSlot, slot 0 indicates
/// that the variable in VariableIdentifier.Symbol is a root, i.e. not nested within another
/// tracked variable. Slots &lt; 0 are illegal.
/// tracked variable. Slots less than 0 are illegal.
/// </summary>
private readonly ArrayBuilder<VariableIdentifier> _variableBySlot;
private readonly ArrayBuilder<VariableIdentifier> _variableBySlot = ArrayBuilder<VariableIdentifier>.GetInstance(1, default);

internal static Variables Create(Symbol? symbol)
{
Expand Down Expand Up @@ -130,15 +128,13 @@ private void Populate(VariablesSnapshot snapshot)

private Variables(NextId nextId, int id, Variables? container, Symbol? symbol)
{
Debug.Assert(container is null || container.Id < nextId.Value);
Debug.Assert(container is null || container.Id < id);
Debug.Assert(id < nextId.Value);
_nextId = nextId;
// PROTOTYPE: Handle > 64K nested methods (distinct ids). See NullableStateTooManyNestedFunctions().
Id = id;
Container = container;
Symbol = symbol;
_variableBySlot = ArrayBuilder<VariableIdentifier>.GetInstance();
_variableBySlot.Add(default); // slot 0 reserved for reachability
}

internal void Free()
Expand Down
74 changes: 32 additions & 42 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2603,16 +2603,15 @@ private void VisitStatementsWithLocalFunctions(BoundBlock block)
var state = TopState();
var startingState = localFunctionState.StartingState;
startingState.ForEach(
static (slot, arg) =>
(slot, variables) =>
Copy link
Member

@333fred 333fred Jan 19, 2021

Choose a reason for hiding this comment

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

Consider marking this lambda static, or making it a regular foreach loop if it can't be. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, there are several locals from the containing method, state, startingState, and localFunc, and state would need to be passed by reference, so it seemed cleaner to allow a closure class instance to be created, especially since this method is only called for local function declarations.


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

Copy link
Member

@333fred 333fred Jan 20, 2021

Choose a reason for hiding this comment

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

Then why not just make this a regular foreach loop? Seems like it would be less lines and not use a side-effecting lambda.


In reply to: 560600004 [](ancestors = 560600004,560551991)

Copy link
Member Author

Choose a reason for hiding this comment

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

Then why not just make this a regular foreach loop?

The method traverses LocalState at all scopes, so a simple foreach is not sufficient.


In reply to: 561181556 [](ancestors = 561181556,560600004,560551991)

{
var (variables, state, localFunc, startingState) = arg;
var symbol = variables[variables.RootSlot(slot)].Symbol;
if (Symbol.IsCaptured(symbol, localFunc))
{
state[slot] = startingState[slot];
}
},
(_variables, state, localFunc, startingState));
_variables);
localFunctionState.Visited = true;

AnalyzeLocalFunctionOrLambda(
Expand Down Expand Up @@ -2681,25 +2680,6 @@ private void AnalyzeLocalFunctionOrLambda(
ImmutableArray<PendingBranch> pendingReturns = RemoveReturns();
RestorePending(oldPending);

var location = lambdaOrFunctionSymbol.Locations.FirstOrNone();
LeaveParameters(lambdaOrFunctionSymbol.Parameters, lambdaOrFunction.Syntax, location);

// Intersect the state of all branches out of the local function
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
var stateAtReturn = this.State;
foreach (PendingBranch pending in pendingReturns)
{
this.State = pending.State;
BoundNode branch = pending.Branch;

// Pass the local function identifier as a location if the branch
// is null or compiler generated.
LeaveParameters(lambdaOrFunctionSymbol.Parameters,
branch?.Syntax,
branch?.WasCompilerGenerated == false ? null : location);

Join(ref stateAtReturn, ref this.State);
}

_snapshotBuilderOpt?.ExitWalker(this.SaveSharedState(), previousSlot);
_variables = _variables.Container!;
this.State = oldState;
Expand Down Expand Up @@ -9603,32 +9583,42 @@ internal LocalStateSnapshot(int id, LocalStateSnapshot? container, BitVector sta
}

[DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
internal sealed class LocalState : ILocalDataFlowState // PROTOTYPE: Pool instances.
internal struct LocalState : ILocalDataFlowState
{
private sealed class Boxed
Copy link
Member

@333fred 333fred Jan 19, 2021

Choose a reason for hiding this comment

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

Boxed [](start = 33, length = 5)

Nit: I think just Box would be a better name. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

We use the same name for TypeWithAnnotations.Boxed.


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

{
internal LocalState Value;
Copy link
Member

@333fred 333fred Jan 19, 2021

Choose a reason for hiding this comment

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

readonly? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

The field needs to be mutable since Value._state is modified.


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


internal Boxed(LocalState value)
{
Value = value;
}
}

internal readonly int Id;
internal LocalState? Container;
private readonly Boxed? _container;
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 21, 2021

Choose a reason for hiding this comment

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

_container [](start = 36, length = 10)

Consider documenting this hierarchy and how it relates to other data structures. #Closed


// The representation of a state is a bit vector with two bits per slot:
// (false, false) => NotNull, (false, true) => MaybeNull, (true, true) => MaybeDefault.
// Slot 0 is used to represent whether the state is reachable (true) or not.
private BitVector _state;

private LocalState(int id, LocalState? container, BitVector state)
private LocalState(int id, Boxed? container, BitVector state)
{
Id = id;
Container = container;
_container = container;
_state = state;
}

internal static LocalState Create(LocalStateSnapshot snapshot)
{
var container = snapshot.Container is null ? null : Create(snapshot.Container);
var container = snapshot.Container is null ? null : new Boxed(Create(snapshot.Container));
return new LocalState(snapshot.Id, container, snapshot.State.Clone());
}

internal LocalStateSnapshot CreateSnapshot()
{
return new LocalStateSnapshot(Id, Container?.CreateSnapshot(), _state.Clone());
return new LocalStateSnapshot(Id, _container?.Value.CreateSnapshot(), _state.Clone());
}

public bool Reachable => _state[0];
Expand All @@ -9649,15 +9639,15 @@ private static LocalState CreateReachableOrUnreachableState(Variables variables,
{
var container = variables.Container is null ?
null :
CreateReachableOrUnreachableState(variables.Container, reachable);
new Boxed(CreateReachableOrUnreachableState(variables.Container, reachable));
int capacity = reachable ? variables.Count : 1;
return new LocalState(variables.Id, container, CreateBitVector(capacity, reachable));
}

public LocalState CreateNestedFunction(Variables variables)
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 22, 2021

Choose a reason for hiding this comment

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

CreateNestedFunction [](start = 30, length = 20)

"CreateNestedFunctionState"? #Closed

{
Debug.Assert(Id == variables.Container!.Id);
return new LocalState(variables.Id, container: this, CreateBitVector(capacity: variables.Count, reachable: true));
return new LocalState(variables.Id, container: new Boxed(this), CreateBitVector(capacity: variables.Count, reachable: true));
}

private static BitVector CreateBitVector(int capacity, bool reachable)
Expand Down Expand Up @@ -9691,7 +9681,7 @@ private bool HasVariable(int id, int index)
{
if (Id > id)
{
return Container!.HasValue(id, index);
return _container!.Value.HasValue(id, index);
}
else
{
Expand All @@ -9714,7 +9704,7 @@ private bool HasValue(int id, int index)
if (Id != id)
{
Debug.Assert(Id > id);
return Container!.HasValue(id, index);
return _container!.Value.HasValue(id, index);
}
else
{
Expand All @@ -9731,7 +9721,7 @@ public void Normalize(NullableWalker walker, Variables variables)
}
else
{
Container?.Normalize(walker, variables.Container!);
_container?.Value.Normalize(walker, variables.Container!);
int start = Capacity;
EnsureCapacity(variables.Count);
Populate(walker, start);
Expand All @@ -9740,7 +9730,7 @@ public void Normalize(NullableWalker walker, Variables variables)

public void PopulateAll(NullableWalker walker)
{
Container?.PopulateAll(walker);
_container?.Value.PopulateAll(walker);
Populate(walker, start: 1);
}

Expand Down Expand Up @@ -9773,7 +9763,7 @@ private NullableFlowState GetValue(int id, int index)
if (Id != id)
{
Debug.Assert(Id > id);
return Container!.GetValue(id, index);
return _container!.Value.GetValue(id, index);
}
else
{
Expand All @@ -9798,7 +9788,7 @@ private void SetValue(int id, int index, NullableFlowState value)
if (Id != id)
{
Debug.Assert(Id > id);
Container!.SetValue(id, index, value);
_container!.Value.SetValue(id, index, value);
}
else
{
Expand All @@ -9812,7 +9802,7 @@ private void SetValue(int id, int index, NullableFlowState value)

internal void ForEach<TArg>(Action<int, TArg> action, TArg arg)
{
Container?.ForEach(action, arg);
_container?.Value.ForEach(action, arg);
for (int index = 1; index < Capacity; index++)
{
action(Variables.ConstructSlot(Id, index), arg);
Expand All @@ -9824,7 +9814,7 @@ internal LocalState GetStateForVariables(int id)
var state = this;
while (state.Id != id)
{
state = state.Container!;
state = state._container!.Value;
}
return state;
}
Expand All @@ -9835,15 +9825,15 @@ internal LocalState GetStateForVariables(int id)
/// <returns></returns>
public LocalState Clone()
{
var container = Container?.Clone();
var container = _container is null ? null : new Boxed(_container.Value.Clone());
return new LocalState(Id, container, _state.Clone());
}

public bool Join(in LocalState other)
{
Debug.Assert(Id == other.Id);
bool result = false;
if (Container is { } && Container.Join(in other.Container!))
if (_container is { } && _container.Value.Join(in other._container!.Value))
Copy link
Member

@333fred 333fred Jan 19, 2021

Choose a reason for hiding this comment

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

{ } [](start = 34, length = 3)

Consider using not null #ByDesign

{
result = true;
}
Expand All @@ -9858,7 +9848,7 @@ public bool Meet(in LocalState other)
{
Debug.Assert(Id == other.Id);
bool result = false;
if (Container is { } && Container.Meet(in other.Container!))
if (_container is { } && _container.Value.Meet(in other._container!.Value))
Copy link
Member

@333fred 333fred Jan 19, 2021

Choose a reason for hiding this comment

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

{ } [](start = 34, length = 3)

Consider using not null #ByDesign

Copy link
Member Author

@cston cston Jan 20, 2021

Choose a reason for hiding this comment

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

I like this syntax since it's shorter.


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

{
result = true;
}
Expand Down Expand Up @@ -9896,7 +9886,7 @@ internal string Dump(Variables variables)

private void Dump(StringBuilder builder, Variables variables)
{
Container?.Dump(builder, variables.Container!);
_container?.Value.Dump(builder, variables.Container!);

for (int index = 1; index < Capacity; index++)
{
Expand Down