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

Add AsyncValueTaskMethodBuilder in support of C# feature #10201

Merged
merged 2 commits into from Jul 22, 2016

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 20, 2016

The C# compiler just merged a feature to allow arbitrary Task-like types in the return position of async methods. In support of that, this commit adds the necessary builder to allow ValueTask<T> to be used in this way.

I've not yet tried this with compiler bits.

cc: @cston, @ljw1004, @mellinoe, @terrajobst, @jaredpar
Related to dotnet/roslyn#12518

@ljw1004
Copy link

@ljw1004 ljw1004 commented Jul 20, 2016

LGTM

@stephentoub stephentoub force-pushed the stephentoub:valuetask_builder branch from 3c3b360 to 509b977 Jul 21, 2016
@stephentoub
Copy link
Member Author

@stephentoub stephentoub commented Jul 21, 2016

Thanks, Lucian. Anyone else want to review before I merge this?

@@ -4,7 +4,7 @@
<PropertyGroup>
<ProjectGuid>{F24D3391-2928-4E83-AADE-B34423498750}</ProjectGuid>
<AssemblyName>System.Threading.Tasks.Extensions</AssemblyName>
<AssemblyVersion>4.0.1.0</AssemblyVersion>
<AssemblyVersion>4.1.0.0</AssemblyVersion>

This comment has been minimized.

@jaredpar

jaredpar Jul 21, 2016
Member

Why this vs. 4.0.2?

This comment has been minimized.

@stephentoub

stephentoub Jul 21, 2016
Author Member

I've been told that adding APIs requires bumping the minor version. @weshaggard, is that correct?

}

[Fact]
public void SetException_BeforeAccessTask_ValueTaskContainsValue()

This comment has been minimized.

@cston

cston Jul 21, 2016
Member

Should the suffix be something like _IsFaulted rather than _ValueTaskContainsValue? Same question for test below.

This comment has been minimized.

@stephentoub

stephentoub Jul 21, 2016
Author Member

Yup, copy/paste error in the name. Will fix. Thanks.

@cston
Copy link
Member

@cston cston commented Jul 21, 2016

LGTM

public void Create_ReturnsDefaultInstance()
{
AsyncValueTaskMethodBuilder<int> b = ValueTask<int>.CreateAsyncMethodBuilder();
Assert.Equal(default(AsyncValueTaskMethodBuilder<int>), b);

This comment has been minimized.

@mellinoe

mellinoe Jul 21, 2016
Contributor

This was a bit confusing to me at first, because AsyncValueTaskMethodBuilder.Create does assign to its _methodBuilder field. But it turns out that it assigns it to default anyways (or technically calls a method returning default and assigns that). I guess we would need to change this test if that ended up returning something else? (There's a comment suggesting we might here)

This comment has been minimized.

@stephentoub

stephentoub Jul 21, 2016
Author Member

I guess we would need to change this test if that ended up returning something else?

Yup

@mellinoe
Copy link
Contributor

@mellinoe mellinoe commented Jul 21, 2016

LGTM

The C# compiler just merged a feature to allow arbitrary Task-like types in the return position of async methods.  In support of that, this commit adds the necessary builder to allow ```ValueTask<T>``` to be used in this way.
@stephentoub stephentoub force-pushed the stephentoub:valuetask_builder branch from 509b977 to 0458fc9 Jul 21, 2016
// ExecutionContext as we don't have access to any of the relevant methods. We also can't
// call _methodBuilder.Start, as then we'll always end up creating a Task<T>. At the moment the
// only solution is to skip the ExecutionContext barrier that we'd want to put in place.
stateMachine.MoveNext();

This comment has been minimized.

@stephentoub

stephentoub Jul 21, 2016
Author Member

@ericeil, not to block this PR, but for subsequently, any thoughts about what to do here?

Outside of mscorlib, the options are relatively limited. If we were to bump up to the latest .NET Standard version, we could use ExecutionContext.Capture/Run to simulate what's being done internally in AsyncTaskMethodBuilder.Start, but that seems unfortunately expensive, at least for desktop, and it means either we don't support earlier standards, or we have multiple builds and end up with different behaviors based on which standard you're targeting.

I'm tempted to leave this as a wart for now. The impact as you know will be that any ExecutionContext-related changes that occur in the async method prior to it yielding will be visible to the caller, but none of the options seem particularly good.

This comment has been minimized.

@ericeil

ericeil Jul 21, 2016
Contributor

Could you just replace the call to MoveNext with this?

AsyncTaskMethodBuilder.Create().Start(ref stateMachine);

This comment has been minimized.

@ericeil

ericeil Jul 21, 2016
Contributor

At least in the CoreCLR implementation, that looks like it would do the trick without allocating unnecessarily.

This comment has been minimized.

@stephentoub

stephentoub Jul 21, 2016
Author Member

Could you just replace the call to MoveNext with this?

I'd convinced myself that the answer was "no", that doing so would cause it to access the AsyncTaskMethodBuilder's Task and force the allocation that all of this is trying to avoid. But looking at it again, Start's whole purpose is really just to wrap the stateMachine's invocation exactly the way we want to, so it should be fine. I'll verify, but... I think this works :)

This comment has been minimized.

@stephentoub

stephentoub Jul 21, 2016
Author Member

Thanks, Eric! Fixed.

@stephentoub
Copy link
Member Author

@stephentoub stephentoub commented Jul 22, 2016

Thanks, all.

@stephentoub stephentoub merged commit ce3e10f into dotnet:master Jul 22, 2016
8 checks passed
8 checks passed
Innerloop CentOS7.1 Debug Build and Test Build finished.
Details
Innerloop CentOS7.1 Release Build and Test Build finished.
Details
Innerloop OSX Debug Build and Test Build finished.
Details
Innerloop OSX Release Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Debug Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release Build and Test Build finished.
Details
Innerloop Windows_NT Debug Build and Test Build finished.
Details
Innerloop Windows_NT Release Build and Test Build finished.
Details
@stephentoub stephentoub deleted the stephentoub:valuetask_builder branch Jul 22, 2016
/// <summary>true if <see cref="_result"/> contains the synchronous result for the async method; otherwise, false.</summary>
private bool _haveResult;
/// <summary>true if the builder should be used for setting/getting the result; otherwise, false.</summary>
private bool _useBuilder;

This comment has been minimized.

@jamesqo

jamesqo Jul 26, 2016
Contributor

Just curious, maybe these two bools could be aggregated into an int instead? If I remember correctly, bools actually take up 4 bytes since the CLR pads them to align field accesses. Maybe it could be changed to a single field (byte, short, or int) that uses flags.

This comment has been minimized.

@stephentoub

stephentoub Jul 26, 2016
Author Member

Making these bools into an int will make it worse if the TResult is two bytes or less; on 32-bit, the TResult and the two bools can then be placed in the same word. Other than that, on both 32-bit and 64-bit, size-wise it's the same, and using an int would just make the code more complicated.

@karelz karelz modified the milestone: 1.1.0 Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.