Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add new System.Threading.Tasks.Extensions library #4857

Merged
merged 2 commits into from
Dec 8, 2015

Conversation

stephentoub
Copy link
Member

This adds a new library to corefx: System.Threading.Tasks.Extensions. For now, this only contains a new type ValueTask<T> and its associated awaiter. More Task-related and async-related types may be added here in the future.

Two open issues:

  1. There is a debate about whether these types should instead be added to System.Threading.Tasks.dll. I've chosen to instead add them to a separate library so that they can be used downlevel to .NET 4.5. This new library has very few dependencies and can be used low in the stack by most any other corefx library we'd want to use this from.
  2. The "ValueTask" name. For now I've gone with "ValueTask", using "Value" as a prefix, but we may choose to subsequently rename it to "TaskValue", with "Value" as a suffix.

Includes tests for both types with 100% line and branch coverage.
cc: @terrajobst, @davidfowl, @AlfredoMS, @mellinoe, @ericstj, @weshaggard
Fixes #4708

<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition="'$(Configuration)' == ''">Debug</Configuration>
Copy link
Member

Choose a reason for hiding this comment

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

Configuration, Platform, OutputType are redundant and with what is in dir.props so they can be cleaned out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I'll remove, thanks.

@weshaggard
Copy link
Member

can be used low in the stack by most any other corefx library we'd want to use this from.

That maybe true for some of the implementations but we will still have to be careful about using it in any public APIs on types that already exist in the full .NET Framework.

As for the naming I don't feel strongly but I slightly prefer ValueTask.

I assume once this gets through you plan to remove the common ValueTask source code that currently exists?

@davidfowl
Copy link
Member

@stephentoub Think it'll be possible to get compiler support added for this type? i.e. make a an async method that returns ValueTask<T>?

/cc @jaredpar @MadsTorgersen

@i3arnon
Copy link

i3arnon commented Dec 7, 2015

@stephentoub It seems that ValueTask has a single combination of an awaiter and a configurable awaitable (in regards to captured context) made possible by ValueTaskAwaiter<TResult> GetAwaiter() => this. This is a different approach than Task which has a "non-configurable" TaskAwaiter and a configurable ConfiguredTaskAwaitable.

This requires a slightly bigger awaiter since it holds the _continueOnCapturedContext on top of the ValueTask being awaited. It also requires always creating a new ConfiguredTaskAwaitable when awaiting a ValueTask (i.e. _value.AsTask().ConfigureAwait(_continueOnCapturedContext).GetAwaiter().OnCompleted(continuation)). even when ConfigureAwait wasn't used.

This overhead seems negligible, but if so then why wasn't this approach used with Task to begin with? Because ValueTask being used together with ConfigureAwait is more probable?

P.S: Are you guys against using C# 6.0 features for corefx? Because almost all of ValueTask's members can be expression-bodied.

For now, it contains just ```ValueTask<TResult>``` and its awaiters.
@stephentoub
Copy link
Member Author

As for the naming I don't feel strongly but I slightly prefer ValueTask.

Me, too 😄

I assume once this gets through you plan to remove the common ValueTask source code that currently exists?

Yes, once there's a package that can be consumed.

@weshaggard, thanks for the review, I've updated the PR with the feedback.

Think it'll be possible to get compiler support added for this type? i.e. make a an async method that returns ValueTask?

@davidfowl, see dotnet/roslyn#7169. That said, for the reasons I call out in the comments (https://github.com/stephentoub/corefx/blob/task_extensions/src/System.Threading.Tasks.Extensions/src/System/Threading/Tasks/ValueTask.cs#L23-L48), i.e. perf and usability, which in turn lead to more perf, I do want to stress again that we should be very thoughtful about where this is used. There are real downsides if used more than it should be. Plus, in situations where ValueTask would be used, a developer is trying hard to avoid as much overhead as possible, and there is some (small) overhead to invoking an async method (if nothing it's unlikely to be inlined), so I'd expect a thoughtful developer in most cases would actually continue to have a non-async entrypoint method that did argument validation, checked for a fast path, and if on the fast path returned a ValueTask<TResult>, otherwise delegated to a private Task<TResult>-returning async method. There will of course be cases where that's not applicable and where there would be a benefit to C# supporting ValueTask as the return type of an async method, but I expect those to be a significant minority.

Awaiters

@i3arnon, I went back and forth on this and actually had both approaches implemented locally. I've switched it over to one with the additional awaitable type.

Are you guys against using C# 6.0 features for corefx?

"Against"? No, I'd very much like to use C# 6 features in corefx, but we're currently blocked from doing so. @weshaggard can explain the limitation and when we expect it to be lifted.

/// <param name="task">The task.</param>
public ValueTask(Task<TResult> task)
{
_task = task;
Copy link

Choose a reason for hiding this comment

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

If task is null here, should we throw?

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 could, but if it's null then the ValueTask will just represent default(TResult). I'm not sure it's worth adding usage error checks for.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd lean toward throwing on the principle that it's not a particularly good way of indicating default(TResult), especially since that can already be done by passing in default(TResult) to the other constructor, and it's always nicer to find oneself thinking "I really should have let that value be valid and not throw" than "I really should have prohibited that value, but now it's a breaking change to throw on it".

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 Task will already be null if one uses default(ValueTask<TResult>), which would be the same as new ValueTask(default(TResult)). It's not clear to me why new ValueTask((Task<TResult>)null) should be different in terms of throwing. There's not a lot of cost to a branch, but it's not free, and if the goal of this type is to help minimize as much cost as possible, it seems odd that we'd add such overheads in to prevent a subset of "incorrect" usage. You could even make the argument that specifying a null task is the same as saying "I don't want to wait for anything".

I don't feel strongly about it, and I'm mostly playing devil's advocate. I just don't know what real value adding such a check would provide. What real and dangerous situation is it helping to prevent?

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, as @JonHanna pointed out it's easier to remove the throw than to add it later, so I've put it in for now.

- Throw an exception when ctor is passed null task
stephentoub added a commit that referenced this pull request Dec 8, 2015
Add new System.Threading.Tasks.Extensions library
@stephentoub stephentoub merged commit 8610df7 into dotnet:master Dec 8, 2015
@stephentoub stephentoub deleted the task_extensions branch December 8, 2015 15:38
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ValueTask to corefx
8 participants