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

Async-streams: Call AsyncIteratorMethodBuilder.Complete #39436

Merged
merged 4 commits into from Oct 22, 2019
Merged
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -176,6 +176,7 @@ IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken token)
if (initialThreadId == /*managedThreadId*/ && state == StateMachineStates.FinishedStateMachine)
{
state = InitialState; // -3
builder = AsyncIteratorMethodBuilder.Create();
disposeMode = false;
result = this;
}
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CodeGen;
using Microsoft.CodeAnalysis.CSharp.Symbols;
@@ -71,6 +71,7 @@ protected override BoundStatement GenerateSetResultCall()

// if (this.combinedTokens != null) { this.combinedTokens.Dispose(); this.combinedTokens = null; } // for enumerables only
// this.promiseOfValueOrEnd.SetResult(false);
// this.builder.Complete();
// return;
// _exprReturnLabelTrue:
// this.promiseOfValueOrEnd.SetResult(true);
@@ -86,6 +87,7 @@ protected override BoundStatement GenerateSetResultCall()
builder.AddRange(
// this.promiseOfValueOrEnd.SetResult(false);
generateSetResultOnPromise(false),
GenerateCompleteOnBuilder(),
F.Return(),
F.Label(_exprReturnLabelTrue),
// this.promiseOfValueOrEnd.SetResult(true);
@@ -102,6 +104,17 @@ BoundExpressionStatement generateSetResultOnPromise(bool result)
}
}

private BoundExpressionStatement GenerateCompleteOnBuilder()
{
// Produce:
// this.builder.Complete();

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

this.builder.Complete(); [](start = 15, length = 24)

It looks like the code below is builder.SetResult(value). #WontFix

return F.ExpressionStatement(
F.Call(
F.Field(F.This(), _asyncMethodBuilderField),
_asyncMethodBuilderMemberCollection.SetResult, // AsyncIteratorMethodBuilder.Complete is the corresponding method to AsyncTaskMethodBuilder.SetResult
ImmutableArray<BoundExpression>.Empty));
}

private void AddDisposeCombinedTokensIfNeeded(ArrayBuilder<BoundStatement> builder)
{
// if (this.combinedTokens != null) { this.combinedTokens.Dispose(); this.combinedTokens = null; } // for enumerables only
@@ -131,6 +144,9 @@ protected override BoundStatement GenerateSetExceptionCall(LocalSymbol exception
_asyncIteratorInfo.SetExceptionMethod,
F.Local(exceptionLocal))));

// this.builder.Complete();
builder.Add(GenerateCompleteOnBuilder());

return F.Block(builder.ToImmutableAndFree());
}

@@ -133,7 +133,7 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
createBuilderMethod: createBuilderMethod,
taskProperty: null,
setException: null, // unused
setResult: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__Complete,
setResult: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__Complete, // AsyncIteratorMethodBuilder.Complete is the corresponding method to AsyncTaskMethodBuilder.SetResult

This comment has been minimized.

Copy link
@agocke

agocke Oct 22, 2019

Contributor

I find this kind of a confusing pattern, but I'm not sure how I would change it #ByDesign

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

The BCL initially had AsyncIteratorMethodBuilder.SetResult (like other builders), but then changed the name since there is no value/result. Still is it the spiritual equivalent... ;-)


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

awaitOnCompleted: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__AwaitOnCompleted,
awaitUnsafeOnCompleted: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__AwaitUnsafeOnCompleted,
start: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__MoveNext_T,
@@ -28,12 +28,12 @@ internal class AsyncMethodToStateMachineRewriter : MethodToStateMachineRewriter
/// <see cref="AsyncVoidMethodBuilder"/>, <see cref="AsyncTaskMethodBuilder"/>, or <see cref="AsyncTaskMethodBuilder{TResult}"/> depending on the
/// return type of the async method.
/// </summary>
private readonly FieldSymbol _asyncMethodBuilderField;
protected readonly FieldSymbol _asyncMethodBuilderField;

/// <summary>
/// A collection of well-known members for the current async method builder.
/// </summary>
private readonly AsyncMethodBuilderMemberCollection _asyncMethodBuilderMemberCollection;
protected readonly AsyncMethodBuilderMemberCollection _asyncMethodBuilderMemberCollection;

/// <summary>
/// The exprReturnLabel is used to label the return handling code at the end of the async state-machine
@@ -51,7 +51,7 @@ internal class AsyncMethodToStateMachineRewriter : MethodToStateMachineRewriter
/// of rewritten return expressions. The return-handling code then uses <c>SetResult</c> on the async method builder
/// to make the result available to the caller.
/// </summary>
private readonly LocalSymbol _exprRetValue;
protected readonly LocalSymbol _exprRetValue;

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

protected [](start = 8, length = 9)

Is this change needed?


private readonly LoweredDynamicOperationFactory _dynamicFactory;

@@ -148,7 +148,7 @@ protected override void GenerateConstructor()
// {
// this.state = state;
// this.initialThreadId = {managedThreadId};
// this.builder = System.Runtime.CompilerServices.AsyncVoidMethodBuilder.Create();
// this.builder = System.Runtime.CompilerServices.AsyncIteratorMethodBuilder.Create();
// }
Debug.Assert(stateMachineType.Constructor is IteratorConstructor);

@@ -164,23 +164,25 @@ protected override void GenerateConstructor()
bodyBuilder.Add(F.Assignment(F.InstanceField(initialThreadIdField), managedThreadId));
}

// this.builder = System.Runtime.CompilerServices.AsyncVoidMethodBuilder.Create();
AsyncMethodBuilderMemberCollection methodScopeAsyncMethodBuilderMemberCollection;
bool found = AsyncMethodBuilderMemberCollection.TryCreate(F, method, typeMap: null, out methodScopeAsyncMethodBuilderMemberCollection);
Debug.Assert(found);

bodyBuilder.Add(
F.Assignment(
F.InstanceField(_builderField),
F.StaticCall(
null,
methodScopeAsyncMethodBuilderMemberCollection.CreateBuilder)));
// this.builder = System.Runtime.CompilerServices.AsyncIteratorMethodBuilder.Create();
bodyBuilder.Add(GenerateCreateAndAssignBuilder());

bodyBuilder.Add(F.Return());
F.CloseMethod(F.Block(bodyBuilder.ToImmutableAndFree()));
bodyBuilder = null;

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

bodyBuilder = null; [](start = 16, length = 19)

Why assign null?

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

This was from a previous code review if I recall correctly. I don't care much, but it does prevent accidental re-use of a free'd pooled instance.

}

private BoundExpressionStatement GenerateCreateAndAssignBuilder()
{
// Produce:
// this.builder = System.Runtime.CompilerServices.AsyncIteratorMethodBuilder.Create();
return F.Assignment(
F.InstanceField(_builderField),
F.StaticCall(
null,
_asyncMethodBuilderMemberCollection.CreateBuilder));
}

protected override void InitializeStateMachine(ArrayBuilder<BoundStatement> bodyBuilder, NamedTypeSymbol frameType, LocalSymbol stateMachineLocal)
{
// var stateMachineLocal = new {StateMachineType}({initialState})
@@ -632,10 +634,23 @@ private void GenerateIAsyncEnumerableImplementation_GetAsyncEnumerator()
GenerateIteratorGetEnumerator(IAsyncEnumerableOfElementType_GetEnumerator, ref managedThreadId, initialState: StateMachineStates.InitialAsyncIteratorStateMachine);
}

protected override BoundStatement GetExtraResetForIteratorGetEnumerator()
protected override void GenerateResetInstance(ArrayBuilder<BoundStatement> builder, int initialState)
{
// disposeMode = false;
return F.Assignment(F.InstanceField(_disposeModeField), F.Literal(false));
// this.state = {initialState};
// this.builder = System.Runtime.CompilerServices.AsyncIteratorMethodBuilder.Create();
// this.disposeMode = false;

builder.Add(
// this.state = {initialState};
F.Assignment(F.Field(F.This(), stateField), F.Literal(initialState)));

builder.Add(
// this.builder = System.Runtime.CompilerServices.AsyncIteratorMethodBuilder.Create();
GenerateCreateAndAssignBuilder());

builder.Add(
// disposeMode = false;
F.Assignment(F.InstanceField(_disposeModeField), F.Literal(false)));
}

protected override void GenerateMoveNext(SynthesizedImplementationMethod moveNextMethod)
@@ -402,20 +402,12 @@ protected SynthesizedImplementationMethod GenerateIteratorGetEnumerator(MethodSy
managedThreadId = MakeCurrentThreadId();

var thenBuilder = ArrayBuilder<BoundStatement>.GetInstance(4);
thenBuilder.Add(
// this.state = {initialState};
F.Assignment(F.Field(F.This(), stateField), F.Literal(initialState)));
GenerateResetInstance(thenBuilder, initialState);

thenBuilder.Add(
// result = this;
F.Assignment(F.Local(resultVariable), F.This()));

var extraReset = GetExtraResetForIteratorGetEnumerator();
if (extraReset != null)
{
thenBuilder.Add(extraReset);
}

if (method.IsStatic || method.ThisParameter.Type.IsReferenceType)
{
// if this is a reference type, no need to copy it since it is not assignable
@@ -473,6 +465,16 @@ protected SynthesizedImplementationMethod GenerateIteratorGetEnumerator(MethodSy
return getEnumerator;
}

/// <summary>
/// Generate logic to reset the current instance (rather than creating a new instance)
/// </summary>
protected virtual void GenerateResetInstance(ArrayBuilder<BoundStatement> builder, int initialState)
{
builder.Add(
// this.state = {initialState};
F.Assignment(F.Field(F.This(), stateField), F.Literal(initialState)));
}

protected virtual BoundStatement InitializeParameterField(MethodSymbol getEnumeratorMethod, ParameterSymbol parameter, BoundExpression resultParameter, BoundExpression parameterProxy)
{
Debug.Assert(!method.IsIterator || !method.IsAsync); // an override handles async-iterators
@@ -481,12 +483,6 @@ protected virtual BoundStatement InitializeParameterField(MethodSymbol getEnumer
return F.Assignment(resultParameter, parameterProxy);
}

/// <summary>
/// Async-iterator methods use a GetAsyncEnumerator method just like the GetEnumerator of iterator methods.
/// But they need to do a bit more work (to reset the dispose mode).
/// </summary>
protected virtual BoundStatement GetExtraResetForIteratorGetEnumerator() => null;

/// <summary>
/// Returns true if either Thread.ManagedThreadId or Environment.CurrentManagedThreadId are available
/// </summary>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.