.NET 6

[slang25]

A few changes to take advantage of .NET 6 & ASP.NET Core 6 for a Giraffe 6 release.

• Target .NET 6
• Removed Ply (you served us well) and used baked in F# 6 support for tasks
• Updated various dependencies
• Utilise some of the minimal APIs for bootstrapping the included app

[forki]

Awesome!

[forki]

Looking good

[kdblocher]

@slang25 Are there any specific edge cases that should be under test because of swapping out Ply with F#6? Task implementations can be flaky in areas one would not expect.

[slang25]

Good question @kdblocher. I wouldn't say flaky, but complex for sure. I consider the Task and the underlying state machine / workflow to be a low level abstraction, which I somewhat understand (not as well as @NinoFloris).

It's an abstraction in that we could use some custom implementation in Giraffe and internalize it (as was done with the very early versions of Giraffe), and consuming apps could use their own implementations of task CEs and it will all "just work".

However it's the implementation of the async workflows, and now F# 6 resumable code and task ce code that really need to be tested, which arguably in F# 6 has been done thoroughly. At least enough to satisfy me.

I'd like to get an updated version of the TechEmpower benchmarks running a beta version of Giraffe with this change, I'd want to see that there is no perf regression and hopefully a nice little boost in numbers.

[forki]

As soon as an updated giraffe hits nuget I'll update the techempower.

WIP here: TechEmpower/FrameworkBenchmarks#6891

[kdblocher]

I'd like to get an updated version of the TechEmpower benchmarks running a beta version of Giraffe with this change, I'd want to see that there is no perf regression and hopefully a nice little boost in numbers.

This would be a good start.

Good question @kdblocher. I wouldn't say flaky, but complex for sure. I consider the Task and the underlying state machine / workflow to be a low level abstraction, which I somewhat understand (not as well as @NinoFloris).

Some colleagues and I did a lot of work around this a couple years ago. Our result on the deep dive is more or less "use it the right way, and there's pretty much one or two right ways to do it, and many ways not to. Hopefully F#6 task takes care of all of this, too, especially with respect to cancellation and exceptions.

It's an abstraction in that we could use some custom implementation in Giraffe and internalize it (as was done with the very early versions of Giraffe), and consuming apps could use their own implementations of task CEs and it will all "just work".

I doubt this is necessary, though it would be nice to be able to support interchangeable task and async scenarios, but that's a lot of work to get the experience right. We don't have HKTs, but SMTCs might work; however, the type checker might spit out unintelligible errors as you sometimes get working with transformers in F#+. (This is obviously out of scope of this PR)

[NinoFloris]

@kdblocher I'm the author of Ply and was involved in some of the testing and design of the resumable code feature (as well as the task implementation built on it) and I'm pretty confident in the implementation.

For the time being the builtin in F# is missing ValueTask return type support so we will still break libraries or people depending on that functionality existing transitively, my opinion is that it's not so important as major versions of Giraffe are only available in one tfm regardless. These projects would all need to add Ply to their direct dependencies instead. Beyond that we may still find some other differences around type inference like dotnet/fsharp#12184 but there haven't been any reports suggesting functional issues.

[dustinmoris]

T H A N K S
😃 👏 🥳

[forki]

is there already a prerelease?

[dustinmoris]

Yes :)
https://github.com/giraffe-fsharp/Giraffe/actions/runs/1443366693

[forki]

Xoxo Dustin Moris Gorski ***@***.***> schrieb am Mi., 10. Nov. 2021, 10:05:

…

Yes :) https://github.com/giraffe-fsharp/Giraffe/actions/runs/1443366693 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#496 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOANECD4I3O34BLLFEMS3ULIYUPANCNFSM5HU7ZIPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[forki]

TechEmpower/FrameworkBenchmarks#6891 looking good

[dsyme]

This is great work. Can anyone link an updated tech-empower result when the figures are available? Just for clarity?

Note there was one known bug in the F# 6 task implementation, now fixed, here: dotnet/fsharp#12359. The fix will be in the next .NET 6/VS 2022 release. The bug is fairly easily avoidable, but still it's probably worth waiting until that fix is through the system before taking this new release of Giraffe out of pre-release.

[kdblocher]

@slang25

However it's the implementation of the async workflows, and now F# 6 resumable code and task ce code that really need to be tested, which arguably in F# 6 has been done thoroughly. At least enough to satisfy me.

I'd like to get an updated version of the TechEmpower benchmarks running a beta version of Giraffe with this change, I'd want to see that there is no perf regression and hopefully a nice little boost in numbers.

@dsyme

Note there was one known bug in the F# 6 task implementation, now fixed, here: dotnet/fsharp#12359. The fix will be in the next .NET 6/VS 2022 release. The bug is fairly easily avoidable, but still it's probably worth waiting until that fix is through the system before taking this new release of Giraffe out of pre-release.

Ditto that this upgrade happening so quickly is great, but it's exactly this sort of thing (dotnet/fsharp#12359) that is the stuff of nightmares for package maintainers.

Do the TechEmpower benchmarks happen to additional provide good runtime coverage so that we could discover more edge-case bugs like this, or does another suite of tests somewhere exist for that?

[dsyme]

@kdblocher Yes, understood.

Do the TechEmpower benchmarks happen to additional provide good runtime coverage so that we could discover more edge-case bugs like this, or does another suite of tests somewhere exist for that?

dotnet/fsharp has an extensive test suite of course, though the more real-world code the better.

[slang25]

I certainly tempted fate with my remarks 😆

The techempower benchmarks have a few simple and well defined use cases, running under great load. I wouldn't expect it to surface this sort of bug (although it could), it's more likely to surface performance regressions.

Our current approach is still appropriate, we have alpha packages out that we can use to trial and gauge our confidence in the changes. We've got a clear signal that there's a messy corner case bug, so we'll hold off. Thanks @dsyme for the heads-up.

[slang25]

@dsyme I think we're waiting on this set of results:
https://tfb-status.techempower.com/results/939169a8-6a56-44ae-9e50-0e714753c445

I'm also looking to enable PGO just like the C# ASP.NET Core versions: TechEmpower/FrameworkBenchmarks#6923

[dsyme]

Our current approach is still appropriate, we have alpha packages out that we can use to trial and gauge our confidence in the changes. We've got a clear signal that there's a messy corner case bug, so we'll hold off. Thanks @dsyme for the heads-up.

Yes, I agree, the current approach is appropriate. Note users always have the opportunity to use Ply or TaskBuilder for their own tasks should another corner-case crop-up.

@dsyme I think we're waiting on this set of results:

Thanks. Please let me know if you see any specific significant regressions, we can likely turn-around a fix fairly quickly. Ply has really impressive performance so I'm expecdting some +/- differences.

[dsyme]

@slang25 Hmmmm I'm concerned there is a major perf slow down here:

Taking the top two here: https://tfb-status.techempower.com/

Before:

After:

We can wait for the full results but that looks problematic. If that persists then we'll need a standalone test that we can profile and understand. We should also verify that it is specifically to do with F# task codegen, and not other changes (since the change is also the move to .NET 6 and maybe some other coding changes to Giraffe)

[dsyme]

This should be removed I think since it's only relevant to Giraffe 5

[slang25]

Yeah, I saw that and hoped it was something to do with it being a partial result, a bit concerned here

[dustinmoris]

Let's see what happens but I think I once observed something similar when all results were not in yet.

[dsyme]

Either way it's probably better to phase this more carefully. First move to net6, then remove ply. Probably best to get that on the works actually.

Also maybe minimise the diff and don't upgrade Newtonsoft.json unless needed

Assuming there is really a problem that is.

If it's tasks I'm sure we'll get to the bottom of it, even if a fix is required in net sdk 6.0.200/dev 17.1. And to be honest it's really amazing to have such end to end benchmarking available.

Note we've done plenty of raw micro benchmarking and things are OK, so if it's a glitch it will not be fundamental, certainly not that much off. I can't imagine anything giving that kind of perf difference.

(The only possible thing might be failed state machine compilation - there's a specific warning (3511) given for that. Is the Giraffe build giving any warnings? We should check the techempower build too and add /warnaserror to both?)

[NinoFloris]

It might just be logging 😅

TechEmpower/FrameworkBenchmarks#6927

[dsyme]

Yay! Thanks for finding that!

[forki]

.NET SDK 6.0.200 is out. Everything looks good now. I think new Giraffe can be released!