[RFC FS-1087, FS-1097, FS-1098] tasks, resumable state machines, inline on parameters

[dsyme]

Continuation of #6634 from a feature branch

RFC FS-1087 resumable code

RFC FS-1097 task builder

RFC FS-1098 inline if lambda

F# Tooling RFC FST-1034 – additional lambda optimizations

Overview

This adds task support to F#. Task code is compiled to "state machines" via a general capability to write resumable code (i.e. here state machines means resumable code in a "MoveNext" method or similar).

The state machine mechanism can be applied to give more efficient stringBuilder, list, array, asynchronous sequences and other generative computations.

• library support for Task task { ... }

• state machine compilation for computation expressions

• support for using task-like tasks (value tasks etc.)

• support for configurable tasks (half done)

• lift limitations on sequence and task state machines for match and other constructs

• support for defining value tasks

• establish what to do about reflective invocations of task code

• check applicability of state machine mechanism for sync { ... }

• check applicability of state machine mechanism for list { ... }

• check applicability of state machine mechanism for option { ... }

• check applicability of state machine mechanism for taskOption { ... }

• check applicability of state machine mechanism for taskSeq { ... }

• validity checks for well-formed state machine code (first part)

• check performance of state machine mechanism for sync { ... }

• check performance of state machine mechanism for option { ... }

• check performance of state machine mechanism for list { ... }

• RFC is drafted (early draft)

• Implementation matches the updates in the RFC

• diff minimized and cleanup factored out

• Implementation is green

• Systematic testing done for resumable state machines and tasks

• Performance checked, still looking good.

• check performance of state machine mechanism for taskSeq { ... }

• add LanguageFeature and Experimental

• validity checks for well-formed state machine code (testing and more needed)

Testing:

• test we have proper debug support (stack, breakpoints, stepping) equivalent to C#
• add test for the "TODO" case for sequence expressions that has been lifted as part of this PR

We won't add the support until all of the above are done. We are starting with TaskBuilder.fs as a reference library implementation to help define semantics.

Performance Status

Systematic perf testing of task { ... } is required.

Some benchmarks are at tests\fsharp\perf\tasks in the PR. Please help improve this.

Currently compile and run with:

msbuild tests\fsharp\perf\tasks\FS\TaskPerf.fsproj /p:Configuration=Release 
dotnet artifacts\bin\TaskPerf\Release\netcoreapp3.1\TaskPerf.dll

After building a new Debug compiler can run with

msbuild tests\fsharp\perf\tasks\FS\TaskPerf.fsproj /p:Configuration=Release /p:FscToolPath=C:\GitHub\dsyme\fsharp\artifacts\bin\fsc\Debug\net472

Here are results at last run

https://github.com/dotnet/fsharp/blob/feature/tasks/BenchmarkDotNet.Artifacts/results/TaskPerf.Benchmarks-report-github.md

[cartermp]

Given the heaviness of this change (it's honestly quite a tricky mechanism), I wonder if it would be good to split out task support into a small PR such that it's pretty much the same as a built-in TaskBuilder.fs. That would be a fairly low-risk addition to FSharp.Core that I think we'd be able to get into the F# 4.7/.NET Core 3.0 release. Would it be possible to do that and stage state machine generation (also generalized!) for a future release?

[forki]

What would this give us? Using TaskBuilder.fs from nuget works fine, right? So I think the only thing we do is pinning the API. This may put us in situation where we will deal with breaking changes if we detect a problem with the state machines later. Also: I still think it's important to remove that implicit conversion from async to task. Phillip Carter <notifications@github.com> schrieb am Do., 23. Mai 2019, 04:45:

…

Given the heaviness of this change (it's honestly quite a tricky mechanism), I wonder if it would be good to split out task support into a small PR such that it's pretty much the same as a built-in TaskBuilder.fs. That would be a fairly low-risk addition to FSharp.Core that I think we'd be able to get into the F# 4.7/.NET Core 3.0 release. Would it be possible to do that and stage state machine generation (also generalized!) for a future release? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6811?email_source=notifications&email_token=AAAOANGK7HOBKI6QXJPCBODPWYALHA5CNFSM4HOTO3W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWA5AAA#issuecomment-495046656>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAOANF562INBBDVOD7UU5DPWYALHANCNFSM4HOTO3WQ> .

[cartermp]

@forki Yes, but by not being built-in, many (most?) will not use it and still assume you need a very allocation-heavy approach to asynchronous programming in F#. As it stands, this PR is too complex (and incomplete) enough to make it into F# 4.7/.NET Core 3.0, so the next opportunity to do what this accomplishes would be probably next year. I'd like to get something good in here that's low-risk and allows people to start adopting the built-in task CE in the interim.

[NinoFloris]

@cartermp TaskBuilder is the epitome of binary incompatibility. I would strongly advise against including any task CE into FSharp.Core while this PR matures.

Same goes for Ply actually. There I did spend some effort to prevent leaking internal types and implementation details but I still have to call into the entrypoint of it all. So now Ply.TplPrimitives.Binder<'u>.Await is forever expected to be there.
(https://github.com/crowded/ply/blob/master/Ply.fs#L373-L387)
(There's also the monad type a CE needs that should be kept compatible too)

All task CE implementations have to do SRTP (i.e. inlining) in their main codepaths which just means you can't prevent those issues at all.

Better to leave the ecosystem stable under TaskBuilder.fs 2.0 that's being referenced everywhere now (and is pulled in transitively for most users as well) and wait a while for this PR to settle.

[forki]

Yes. From technical standpoint I think we should keep it as is until this one is really ready. What MS (and the broader community) can do is to give some kind of unofficial stamp of approval to the project. In some sense the approved RFC is already this Nino Floris <notifications@github.com> schrieb am Do., 23. Mai 2019, 19:45:

…

@cartermp <https://github.com/cartermp> TaskBuilder is the epitome of binary incompatibility. I would strongly advise against including any task CE into FSharp.Core while this PR matures. Same goes for Ply actually. There I did spend some effort to prevent leaking internal types and implementation details but I still have to call into the entrypoint of it all. So now Ply.TplPrimitives.Binder<'u>.Await is forever expected to be there. (https://github.com/crowded/ply/blob/master/Ply.fs#L373-L387) (There's also the monad type a CE needs that should be kept compatible too) All task CE implementations have to do SRTP (i.e. inlining) in their main codepaths which just means you can't prevent those issues at all. Better to leave the ecosystem stable under TaskBuilder.fs 2.0 that's being referenced everywhere now (and is pulled in transitively for most users as well) and wait a while for this PR to settle. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6811?email_source=notifications&email_token=AAAOANF5GHE3RQGHOB7D64DPW3J2BA5CNFSM4HOTO3W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWC7DAY#issuecomment-495317379>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAOANC5OXD7JYZHYQR6LDTPW3J2BANCNFSM4HOTO3WQ> .

[pkese]

Can this new task support be made available as a Nuget library, or is it dependent on compiler changes (and will have to wait until next year in any case)?

The meta-question here is: if pushing new tasks into FSharp.Core causes them not appear for another year, then why not in the meanwhile distribute it as a standalone library and stick it into FSharp.Core at a later point.

[forki]

@pkese it's already on nuget: https://www.nuget.org/packages/TaskBuilder.fs - the problem that @cartermp describes (I think) is that it's not official MS stuff (yet) and therefore way fewer people know about it and even fewer try to use it.

[forki]

@pkese and of course this PR additionally ships compiler optimizations that can't be shipped as a NuGet package

[davidglassborow]

Given the heaviness of this change (it's honestly quite a tricky mechanism), I wonder if it would be good to split out task support into a small PR such that it's pretty much the same as a built-in TaskBuilder.fs. That would be a fairly low-risk addition to FSharp.Core that I think we'd be able to get into the F# 4.7/.NET Core 3.0 release. Would it be possible to do that and stage state machine generation (also generalized!) for a future release?

@cartermp when are the timescales to get things included for 4.7/core 3 ? Are your concerns primarily around the additional statemachine logic in the compiler just for Tasks, or changes to build Seq, List, etc. on top of the statemachine feature ?

[cartermp]

@davidglassborow We want to freeze the F# language for the .NET Core 3 release by July, if possible. Later can always be done, but we need to get into the habit of freezing earlier and allowing more time for testing and for issues to be found in previews rather than a release.

As for this PR, it's very complex, incomplete from a code standpoint, and missing a spec we can check the implementation against. This will make thorough review extremely difficult to do, and considering we have other things to do for the release (other features, Default Interface Member interop, perf work, .NET Core FSI with package management) I'd much rather nothing is shipped than something with a tail of bugs that we could have easily caught in advance.

[dsyme]

Given the heaviness of this change (it's honestly quite a tricky mechanism), I wonder if it would be good to split out task support into a small PR such that it's pretty much the same as a built-in TaskBuilder.fs.

We decided against that in planning - we would only add task support if it is precisely on-par with C# from a performance perspective. I think we should stick to this.

I'm also loathe to add a variation of the feature that might in any way diverge from a type checking or semantic point of view. For example, the task { .. } builder methods for TaskBuilder.fs have subtly different type signatures, as well as different struct sizes in the return type. I think otherwise it is semantically identical but if we miss any divergence then we will have real difficulty in later optimizing the mechanism.

[baronfel]

How do these state-machine CEs interact with the restriction on using CE's when you also need to use protected members of a base class? Right now we can't use async, task, etc from overrides of a parent class. As a real-world example, take the class UserClaimsPrincipalFactory<TUser,TRole> from Asp.Net Core Identity:

implementation of `UserClaimsPrincipalFactory`

namespace Microsoft.AspNetCore.Identity
{
  /// <summary>
  /// Provides methods to create a claims principal for a given user.
  /// </summary>
  /// <typeparam name="TUser">The type used to represent a user.</typeparam>
  /// <typeparam name="TRole">The type used to represent a role.</typeparam>
  public class UserClaimsPrincipalFactory<TUser, TRole> : UserClaimsPrincipalFactory<TUser>
    where TUser : class
    where TRole : class
  {
    /// <summary>
    /// Initializes a new instance of the <see cref="T:Microsoft.AspNetCore.Identity.UserClaimsPrincipalFactory`2" /> class.
    /// </summary>
    /// <param name="userManager">The <see cref="T:Microsoft.AspNetCore.Identity.UserManager`1" /> to retrieve user information from.</param>
    /// <param name="roleManager">The <see cref="T:Microsoft.AspNetCore.Identity.RoleManager`1" /> to retrieve a user's roles from.</param>
    /// <param name="options">The configured <see cref="T:Microsoft.AspNetCore.Identity.IdentityOptions" />.</param>
    public UserClaimsPrincipalFactory(
      UserManager<TUser> userManager,
      Microsoft.AspNetCore.Identity.RoleManager<TRole> roleManager,
      IOptions<IdentityOptions> options)
      : base(userManager, options)
    {
      if (roleManager == null)
        throw new ArgumentNullException(nameof (roleManager));
      this.RoleManager = roleManager;
    }

    /// <summary>
    /// Gets the <see cref="T:Microsoft.AspNetCore.Identity.RoleManager`1" /> for this factory.
    /// </summary>
    /// <value>
    /// The current <see cref="T:Microsoft.AspNetCore.Identity.RoleManager`1" /> for this factory instance.
    /// </value>
    public Microsoft.AspNetCore.Identity.RoleManager<TRole> RoleManager { get; private set; }

    /// <summary>Generate the claims for a user.</summary>
    /// <param name="user">The user to create a <see cref="T:System.Security.Claims.ClaimsIdentity" /> from.</param>
    /// <returns>The <see cref="T:System.Threading.Tasks.Task" /> that represents the asynchronous creation operation, containing the created <see cref="T:System.Security.Claims.ClaimsIdentity" />.</returns>
    protected override async Task<ClaimsIdentity> GenerateClaimsAsync(TUser user)
    {
      UserClaimsPrincipalFactory<TUser, TRole> principalFactory = this;
      // ISSUE: reference to a compiler-generated method
      ClaimsIdentity id = await principalFactory.\u003C\u003En__0(user);
      if (principalFactory.UserManager.SupportsUserRole)
      {
        foreach (string roleName in (IEnumerable<string>) await principalFactory.UserManager.GetRolesAsync(user))
        {
          id.AddClaim(new Claim(principalFactory.Options.ClaimsIdentity.RoleClaimType, roleName));
          if (principalFactory.RoleManager.SupportsRoleClaims)
          {
            TRole byNameAsync = await principalFactory.RoleManager.FindByNameAsync(roleName);
            if ((object) byNameAsync != null)
            {
              ClaimsIdentity claimsIdentity = id;
              claimsIdentity.AddClaims((IEnumerable<Claim>) await principalFactory.RoleManager.GetClaimsAsync(byNameAsync));
              claimsIdentity = (ClaimsIdentity) null;
            }
          }
        }
      }
      return id;
    }
  }
}

If I have a business use case that involves another asynchronous call to fetch data about the user I can't simply override this member and use a task expression at the moment:

Ideal version of adding a claim via some external resource

type SomeOtherthing(params....) = 
  inherit UserClaimsPrincipalFactory<TUser,TRole>(params...)

  override x.GenerateClaimsAsync(user) = task {
    let! baseIdenitity = base.GenerateClaimsAsync(user)
    let! externalLogins = getExternalLoginsForUser user // 'User -> Task<IEnumerable<System.Security.Claim>>
    for login in externalLogins do
      baseIdentity.AddClaim(login)
    return baseIdentity
  }

Instead I have to do a whole bunch of raw task manipulation, or extract logic out to a function and pass in the already-started task from the base class, which isn't ideal if I want to conditionally-execute that base class call.

[NinoFloris]

The 'protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions' error I usually work around by proxying those through private methods that I then call instead.

    member inline private __.UpdatePasswordHashImpl(user, password, validate) =
        base.UpdatePasswordHash(user, password, validate)

I don't think this PR would change a lot about that, AFAIK this rewriting and expansion happens after type checking which would be after that error is determined to apply. To fsc, up until that point, the CE is expecting actual lambdas, capable of allowing such a method to escape.

@dsyme would you be willing to loosen that constraint for CE lambdas or is there something smart we can do there to make at least task and async work more naturally in protected methods?

[dsyme]

@dsyme would you be willing to loosen that constraint for CE lambdas or is there something smart we can do there to make at least task and async work more naturally in protected methods?

IIRC the key thing to address this is to change the IlxGen.fs to generate closure classes for closures occuring in F# class definitions as nested classes of the actual type being defined, which are then allowed to access protected members. But I'd need to double check the details

This wouldn't be part of this PR

[dsyme]

Good news in this branch we now have both state machine compilation for tasks and reflective execution for some tasks (or potentially tasks where state machine compilation has not succeeded for some reason - though we currently give an error on those)

This means we have a technical basis for evaluating quotations of task { .. } code and other state-machine-enabled-computation-expressions that have not gone through state machine compilation.

There are some details to still work out as task { ... } uses SRTP - we need #6810 to complete that part of the picture.

I need to continue to simplify some of the details but there has been considerable simplification on this iteration - and I'm getting more confident the feature is at a place where we want it.

I'll start work on the RFC tomorrow.

[ctaggart]

Dear 🎅, all we want for 🎄 2020 is F# task support. Thanks for continuing to plug away at this @dsyme. 🥂

[dsyme]

Dear 🎅, all we want for 🎄 2020 is F# task support. Thanks for continuing to plug away at this @dsyme. 🥂

Not before xmas, but maybe I can at least get the branch green....

[TIHan]

These are going to be great features for F#. I can't wait to have them in preview :) !

I have some comments and some nits, but not too many. I'm glad how clean the lowering code for state machines was.

[dsyme]

@KevinRansom requested changes 4 days ago

@KevinRansom I've pushed minor changes based on @TIHan's feedback. Now he has approved I believe we can merge this? (You have to clear your "request changes" review to do that)

[vzarytovskii]

@KevinRansom requested changes 4 days ago

@KevinRansom I've pushed minor changes based on @TIHan's feedback. Now he has approved I believe we can merge this? (You have to clear your "request changes" review to do that)

I think I can clear it and we can merge if you're ok with that.

[dsyme]

I think I can clear it and we can merge if you're ok with that.

I just pushed the code review changes, guess we should wait until it's green

[vzarytovskii]

I think I can clear it and we can merge if you're ok with that.

I just pushed the code review changes, guess we should wait until it's green

Sure. I meant that we can merge once it's green.

[vzarytovskii]

Huh, some weird test utilities (CompilerAssert) issues.

Will has reviewed

[En3Tho]

Thanks!