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

Use RunImmediate for better debug stacks #11788

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Use RunImmediate for better debug stacks #11788

merged 2 commits into from
Jul 6, 2021

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Jul 6, 2021

Addresses internal engineering issue #11754

Async.RunSynchronously starts the async on a thread pool thread. There is a suggestion to add Async.RunImmediate that never does this. fsharp/fslang-suggestions#1042. This can be defined in terms of Async.StartWithContinuations (which should really be called Async.StartImmediateWithContinuations)

This defines and uses Async.RunImmediate internally in our compiler/service implementation and testing since it gives much better synchronous stacks.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 6, 2021

@TIHan THis is ready

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Looks good, this should help our stacks a little bit, especially in testing.

Only comment I have is that we have duplicate RunImmediate implementations; shouldn't we just have one?
I think maybe two will be ok, one for compiler internals and one for all of testing.

@@ -17,6 +18,19 @@ open NUnit.Framework

module Utilities =

type Async with
static member RunImmediate (computation: Async<'T>, ?cancellationToken ) =
Copy link
Member

Choose a reason for hiding this comment

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

Shall we consolidate this one with the default one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default one is not public (not added to FSharp.Core)

@dsyme dsyme merged commit d48369f into dotnet:main Jul 6, 2021
@dsyme
Copy link
Contributor Author

dsyme commented Jul 6, 2021

Only comment I have is that we have duplicate RunImmediate implementations; shouldn't we just have one? I think maybe two will be ok, one for compiler internals and one for all of testing.

I'm sure there was a way to consolidate these a bit - however I couldn't find it immediately. We have one internal in the FSharp.Compiler.Service.dll and one in each test suite. TBH the easiest way to consoldiate is probably to add this method to FSharp.Core

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* Use RunImmediate for better debug stacks

* fix build

Co-authored-by: Don Syme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants