Skip to content

[WIP] Remove trampoline/hijack protection in async.fs and rely on tailcalls being taken - #11807

Closed
dsyme wants to merge 21 commits into
dotnet:mainfrom
dsyme:nojack
Closed

[WIP] Remove trampoline/hijack protection in async.fs and rely on tailcalls being taken#11807
dsyme wants to merge 21 commits into
dotnet:mainfrom
dsyme:nojack

Conversation

@dsyme

@dsymedsyme commented Jul 8, 2021

Copy link
Copy Markdown
Contributor

I'm mostly just curious what CI says

The async implementation historically included trampoline/hijack protection so that, after a certain number of operations running an async, the continuation would be hijacked and execution restarted in the trampoline installed at the base of the callstack

This was needed largely because tailcalls could not be guaranteed, especially in partial trust code for .NET Framework 2.0, when tailcalls were often disabled, or if tailcalls were not taken on implementations of .NET like Silverlight.

For .NET Core, we should not need this, as tailcalls are now much more reliable. So this is an experiment to remove the hijack protection altogether and see what passes and what doesn't.

The trampoline is also used when exceptions happen when running asyncs, so is not removed entirely.

@dsyme

dsyme commented Jul 9, 2021

Copy link
Copy Markdown
ContributorAuthor

This uncovered a case where the F# compiler is incorrectly deciding to not emit a tailcall

I will look into this and fix it and then see if this goes green (again, out of curiosity, and to check our tailcall codegen)

@dsymedsyme mentioned this pull request Jul 10, 2021
@dsyme

Copy link
Copy Markdown
ContributorAuthor

It's interesting that this passes now. It's possible we should use this on .net core. I'll do some perf measurements.

@KevinRansom

Copy link
Copy Markdown
Contributor

@dsyme, do we need to keep this active?

@dsyme

Copy link
Copy Markdown
ContributorAuthor

Yes, we should merge it at some point. I'm not sure what other validation to do though. I'll get back to it.

@edgarfgp

Copy link
Copy Markdown
Contributor

While learning F# and about tailcalls in this Article . I came across this PR . I was wondering is this going be merged and how can affect performance?

@dsymedsyme changed the title Experiment: remove trampoline/hijack protection in async.fs and rely on tailcalls being taken[WIP] Remove trampoline/hijack protection in async.fs and rely on tailcalls being takenJul 12, 2022
@dsyme

Copy link
Copy Markdown
ContributorAuthor

I was wondering is this going be merged and how can affect performance?

In short, we don't know the answers to these. Merging is probably unlikely without specific concrete known benefits beyond code simplification, because this makes us considerably more exposed to glitches in tailcall implementations.

@NinoFloris

Copy link
Copy Markdown
Contributor

The TPL does stack probing to see if it should restart on a fresh stack, couldn't we do that here?

https://github.com/dotnet/runtime/blob/d3af4921f36dba8dde35ade7dff59a3a192edddb/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L3401

It's a netstandard2.1 api but I can't imagine FSharp.Core being netstandard2.0 forever.
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimehelpers.tryensuresufficientexecutionstack?view=net-6.0

Some of the BCL uses a wrapper class like StackGuard and others directly use the api, like the TPL.
https://github.com/dotnet/runtime/search?q=TryEnsureSufficientExecutionStack
https://github.com/dotnet/runtime/search?q=StackGuard

@vzarytovskii
vzarytovskii marked this pull request as draft September 6, 2022 17:33
@vzarytovskiivzarytovskii added this to the January-2023 milestone Jan 9, 2023
@KevinRansomKevinRansom modified the milestones: April-2023, BacklogMay 9, 2023
@KevinRansomKevinRansom removed their assignment May 9, 2023
@dsymedsyme closed this Oct 28, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants

@dsyme@KevinRansom@edgarfgp@NinoFloris@vzarytovskii