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

Consider StartImmediateAsTask instead of StartAsTask to prevent a thread hop #135

Closed
abelbraaksma opened this issue Dec 18, 2022 · 3 comments · Fixed by #191
Closed

Consider StartImmediateAsTask instead of StartAsTask to prevent a thread hop #135

abelbraaksma opened this issue Dec 18, 2022 · 3 comments · Fixed by #191
Labels
performance Performance questions, improvements and fixes topic: taskseq-ce Related to the taskseq computation expression builders or overloads
Milestone

Comments

@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 18, 2022

In PR #114 (adding support for Async in Bind), @bartelink raised the question whether it would be better to use StartImmediateAsTask instead of StartAsTask.

Specifically, it is about this part:

let mutable awaiter =
    Async
        .StartAsTask(asyncSource, cancellationToken = sm.Data.cancellationToken)
        .GetAwaiter()

The current approach was taken to mimic the same approach taken for task in F# Core: https://github.com/dotnet/fsharp/blob/d91b6c5c97accf363d135d1f99816410da4ec341/src/FSharp.Core/tasks.fs#L462, basically:

member inline this.Bind(computation: Async<'TResult1>, continuation: ('TResult1 -> TaskCode<'TOverall, 'TResult2>)) : TaskCode<'TOverall, 'TResult2> =
    this.Bind(Async.StartAsTask computation, continuation)

For comparison, see what was done in async2, which was poised to be the new async based on resumable code, like task is, but unlike task cold-started like the current async.

So, we have two scenarios:

  • use StartAsTask to get similar semantics as the task CE, but this causes a thread hop
  • use StartImmediateAsTask, which uses the current context

I'm not aware of any downside of using the latter. It definitely saves a thread switch in the general case (note that the thread pool may not always cause a physical thread switch).

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Dec 19, 2022

This seems relevant as well: dotnet/fsharp#11043 (comment)

That comment by @dsyme basically suggests not to use StartAsStask. Tbh, I didn't know it started in the thread pool, we have Async.Start already, I figured, as a task, it behaves like task: stay in the same context.

I figured, maybe just report as a bug to F#: dotnet/fsharp#14490

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Dec 19, 2022

Thanks for bringing this up @bartelink. It required some digging, and writing a context-switching test case, but I'm now convinced this is a bad situation to be in (tbh, as Don mentioned already, these methods should be differently named). Will write a PR to fix it.

@abelbraaksma
Copy link
Member Author

The thread hop in F# Core task has been fixed. We should follow suit. See dotnet/fsharp#14499.

@abelbraaksma abelbraaksma added investigation Requires further or deeper investigation discussion topic: taskseq-ce Related to the taskseq computation expression builders or overloads performance Performance questions, improvements and fixes labels Oct 29, 2023
@abelbraaksma abelbraaksma removed investigation Requires further or deeper investigation discussion labels Nov 2, 2023
@abelbraaksma abelbraaksma added this to the v0.4.0 milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance questions, improvements and fixes topic: taskseq-ce Related to the taskseq computation expression builders or overloads
Projects
None yet
1 participant