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

Clear hoisted locals when disposing iterator and async-iterator state machines #75908

Merged
merged 20 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,22 @@ internal AsyncIteratorMethodToStateMachineRewriter(

return (asyncDispatch != null) ? F.Block(asyncDispatch, iteratorDispatch) : iteratorDispatch;
}

protected override BoundStatement GenerateHoistedLocalsCleanup(ImmutableArray<StateMachineFieldSymbol> hoistedLocals)
{
// We need to clean nested hoisted local variables too (not just top-level ones)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024

Choose a reason for hiding this comment

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

We need to clean nested hoisted local variables too (not just top-level ones)

This isn't obvious. It looks like there are two call sites of this method and it is not clear why we want to have this behavior for all of them. Instead of completely ignoring the passed argument, consider adjusting the call sites to construct the right set of locals to clear. We can discuss offline in more details. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior could be a source of bugs in the future, since the method doesn't do what a reasonable dev would expect it to do.

// as they are not cleaned when exiting a block if we exit using a `yield break`
// or if the caller interrupts the enumeration after we reached a `yield return`.
// So we clean both top-level and nested hoisted local variables
return GenerateAllHoistedLocalsCleanup();
}
#nullable disable

protected override BoundStatement GenerateSetResultCall()
{
// ... _exprReturnLabel: ...
// ... this.state = FinishedState; ...
// ... hoisted locals cleanup ...

// if (this.combinedTokens != null) { this.combinedTokens.Dispose(); this.combinedTokens = null; } // for enumerables only
// _current = default;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
Expand Down Expand Up @@ -258,7 +259,7 @@ protected BoundCatchBlock GenerateExceptionHandling(LocalSymbol exceptionLocal,
isSynthesizedAsyncCatchAll: true);
}

protected BoundStatement GenerateHoistedLocalsCleanup(ImmutableArray<StateMachineFieldSymbol> hoistedLocals)
protected virtual BoundStatement GenerateHoistedLocalsCleanup(ImmutableArray<StateMachineFieldSymbol> hoistedLocals)
jcouv marked this conversation as resolved.
Show resolved Hide resolved
{
var builder = ArrayBuilder<BoundStatement>.GetInstance();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,11 @@ internal void GenerateMoveNextAndDispose(BoundStatement body, SynthesizedImpleme
if (rootFrame.knownStates == null)
{
// nothing to finalize
F.CloseMethod(F.Return());
var disposeBody = F.Block(
GenerateAllHoistedLocalsCleanup(),
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024

Choose a reason for hiding this comment

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

GenerateAllHoistedLocalsCleanup(),

Are there tests observing effect of this on IL? #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.

Yes, see the diff between commits 1 and 2 for VerifyIL differences.
This change affects all iterator scenarios without try/finally (such as AddVariableCleanup_StringLocal), whereas the change below affects scenarios with try/finally (such as AddVariableCleanup_NestedStringLocal_InTryFinally)

F.Return());

F.CloseMethod(disposeBody);
}
else
{
Expand All @@ -171,6 +175,7 @@ internal void GenerateMoveNextAndDispose(BoundStatement body, SynthesizedImpleme
ImmutableArray.Create<LocalSymbol>(stateLocal),
F.Assignment(F.Local(stateLocal), F.Field(F.This(), stateField)),
EmitFinallyFrame(rootFrame, state),
GenerateAllHoistedLocalsCleanup(),
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024

Choose a reason for hiding this comment

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

GenerateAllHoistedLocalsCleanup(),

Are there tests observing effect of this on IL? #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.

Yes. See AddVariableCleanup_NestedStringLocal_InTryFinally

F.Return());

F.CloseMethod(disposeBody);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ internal abstract class MethodToStateMachineRewriter : MethodToClassRewriter
/// Note that there is a dispatch occurring at every try-finally statement, so this
/// variable takes on a new set of values inside each try block.
/// </summary>
private Dictionary<LabelSymbol, List<StateMachineState>> _dispatches = new();
private Dictionary<LabelSymbol, List<StateMachineState>> _dispatches = new Dictionary<LabelSymbol, List<StateMachineState>>();

/// <summary>
/// A pool of fields used to hoist locals. They appear in this set when not in scope,
Expand All @@ -75,11 +75,6 @@ internal abstract class MethodToStateMachineRewriter : MethodToClassRewriter
/// </summary>
private int _nextHoistedFieldId = 1;

/// <summary>
/// Used to enumerate the instance fields of a struct.
/// </summary>
private readonly EmptyStructTypeCache _emptyStructTypeCache = EmptyStructTypeCache.CreateNeverEmpty();

/// <summary>
/// The set of local variables and parameters that were hoisted and need a proxy.
/// </summary>
Expand Down Expand Up @@ -434,6 +429,50 @@ private void AddVariableCleanup(ArrayBuilder<BoundExpression> cleanup, FieldSymb
}
}

#nullable enable
protected BoundBlock GenerateAllHoistedLocalsCleanup()
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024

Choose a reason for hiding this comment

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

GenerateAllHoistedLocalsCleanup

This code looks very different from original implementation of GenerateHoistedLocalsCleanup. Is it based on some other existing code? Also, isn't there a more direct and robust way to enumerate fields used for hoisting? I mean without going through locals, interpreting their replacements, etc. If not, perhaps we can build the list in advance, when the fields are added. #Closed

{
var variableCleanup = ArrayBuilder<BoundExpression>.GetInstance();
var alreadyCleaned = PooledHashSet<string>.GetInstance();
foreach (var symbol in HoistedVariables)
jcouv marked this conversation as resolved.
Show resolved Hide resolved
{
if (symbol is LocalSymbol
&& proxies.TryGetValue(symbol, out CapturedSymbolReplacement? replacement))
{
Debug.Assert(replacement is CapturedToStateMachineFieldReplacement or CapturedToExpressionSymbolReplacement);

if (replacement is CapturedToStateMachineFieldReplacement fieldReplacement)
{
addVariableCleanupOnce(variableCleanup, fieldReplacement.HoistedField, alreadyCleaned);
}
else if (replacement is CapturedToExpressionSymbolReplacement expressionReplacement)
{
foreach (var field in expressionReplacement.HoistedFields)
jcouv marked this conversation as resolved.
Show resolved Hide resolved
{
addVariableCleanupOnce(variableCleanup, field, alreadyCleaned);
}
}
}
}

var result = F.Block(variableCleanup.SelectAsArray((e, f) => (BoundStatement)f.ExpressionStatement(e), F));

variableCleanup.Free();
alreadyCleaned.Free();

return result;

void addVariableCleanupOnce(ArrayBuilder<BoundExpression> variableCleanup, FieldSymbol field, PooledHashSet<string> alreadyCleaned)
{
// Hoisted fields may be re-used, so we can skip those that were cleared already
if (alreadyCleaned.Add(field.Name))
{
AddVariableCleanup(variableCleanup, field);
}
}
}
#nullable disable

private StateMachineFieldSymbol GetOrAllocateReusableHoistedField(TypeSymbol type, out bool reused, LocalSymbol local = null)
{
ArrayBuilder<StateMachineFieldSymbol> fields;
Expand Down
Loading