fsharp
fsharp copied to clipboard
dotnet
Feat: Implement builder for valuetask
This is a proposition of implementation of a valuetask computation expression.
A explained in https://github.com/fsharp/fslang-suggestions/issues/1244 :
The advantages of making this adjustment to F# are:
More efficient and faster code Far lower memory impact No need for wrapping Task in ValueTask (which is not easy to get right and is verbose) More straightforward in case of libraries and frameworks that use ValueTask The disadvantages of making this adjustment to F# are :
A new ValueTaskBuilder implementation quite similar to TaskBuilder.
This implementation rely on .IsCompleted to avoid accessing the underlying Task when not needed.
As noticed in tests, an helper function could be useful to make a ValueTask from a ValueTask<unit>. Since `ValueTask<'t>' is a struct, it cannot be cast. It could be implemented as:
let toValueTask (task: ValueTask<unit>) : ValueTask =
if task.IsCompleted then
ValueTask.CompletedTask
else
task.AsTask() |> ValueTask
But I don't know yet what would be the best name, and where to put it.
sure,
for now it can bind on ValueTask, Task, Async and awaitables. (I put Task in medium priority with Async to avoid resolution problems with failwith)
I don't know what triggers this error of duplicate compensation@17 .. Seems related to the code generation in .TryFinally, but I can't see why it's failing only in specific builds...
btw, who is in charge of writing the RFC ?
It can be up to anyone, in most of the cases it's whoewer imlements the feature. The flow is usually Suggestion -> Approval -> RFC (+iterations) -> Implementation. Last two can be in parallel, to not waste time.
To simplify the review process, can you please explain how the implementation was created ? Did you take the taskbuilder and adjust all pieces to work with ValueTask, or are there any outstanding changes compared to the existing taskbuilder?
I tried to make as few changes as possible compared to TaskBuilder:
- I renamed types in the file to add a Value prefix
- used
AsyncValueTaskMethodBuilderfrom the Bcl instead ofAsyncTaskMethodBuiler(this class is available only in netstandard2.1, the reason for the#if) - Changed the argument and return types to ValueTask
- removed implementations related to backgroundtask
Then where new tasks are created, I used the IsCompleted property to return synchronously, and fallback to task creation if not completed.
I also added a Bind for task, added to the Medium Priority to avoid conflict in cases of return failwith "..."
Last part is adding all the test by copying the tests from task, and adapting to valuetask .
In general, I don't think it is a good idea to split codegen for FSharp.Core that much in case of ns21 as long as we still target ns20. It is very confusing to the users.
Might be a better idea to have it as separate package, once we start separating FSharp.Core cc @KevinRansom
Actually, ValueTask is available only in netstandard2.1, and is used more and more in recent frameworks. Having it OOTB in F# seemed sensisble.
Actually, ValueTask is available only in netstandard2.1, and is used more and more in recent frameworks. Having it OOTB in F# seemed sensisble.
What I meant is - it is confusing for users to have the whole builder api in FSharp.Core - netstandard2.1, and not have it in netstandard2.0.
We never did that big of the surface area difference yet (biggest one so far is TryFinally in Task available only in ns21, if I'm not mistaken).
Yes I understand. But at some point some new features will appear only in net6. 0+... Having them in Fsharp.Core will have to follow.
Yes I understand. But at some point some new features will appear only in net6. 0+... Having them in Fsharp.Core will have to follow.
I wonder if there's a better way for us to deliver those. Maybe layer corelib as FSharp.Core and all .Control. packages separately, and give those the same treatment we have for FSharp.Core currently.
Yes that could be a nice way to do it.
Still, since the type ValueTask<'t> itself is only available in netstandard2.1 and compatible frameworks (Core 2.1, Core 2.2, Core 3.0, Core 3.1, 5, 6, 7), I'm not sure anyone using netstandard2.0 or less will find it strange that the CE doesn't exist. It make just no sense on this framework.
Like I would not expect task to exist on super-old frameworks where Task did not exist.
https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask?view=net-7.0
What I meant is - it is confusing for users to have the whole builder api in FSharp.Core - netstandard2.1, and not have it in netstandard2.0.
Why not netstandard2.0? The type is available from ns1.0 using System.Threading.Tasks.Extensions nuget package so ns2.0 or net461 should pose no problem.
What I meant is - it is confusing for users to have the whole builder api in FSharp.Core - netstandard2.1, and not have it in netstandard2.0.
Why not netstandard2.0? The type is available from ns1.0 using System.Threading.Tasks.Extensions nuget package so ns2.0 or net461 should pose no problem.
Core library doesn't have any external dependencies, nor do we want it to have any at this point without proper design (to either properly separate it to multiple packages, or bump minimal version, or split codegen, or some secret 4th option).
Going to close it for now, until we have a better story of how to deliver layered core library for basic ns2.0 functionality and newer stuff (https://github.com/fsharp/fslang-suggestions/issues/1244#issuecomment-2161839536)