.NET 6 by slang25 · Pull Request #496 · giraffe-fsharp/Giraffe · GitHub

Copy link

Member

forki commented 10 days ago •

Awesome!

forki commented 10 days ago

Looking good

kdblocher commented 9 days ago

@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 commented 9 days ago

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 commented 9 days ago •

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

WIP here: TechEmpower/FrameworkBenchmarks#6891

kdblocher commented 9 days ago

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 commented 9 days ago

@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.

forki commented 9 days ago

is there already a prerelease?

forki commented 9 days ago

dsyme commented 2 days ago

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 commented 2 days ago

@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 commented 2 days ago

@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 commented 2 days ago

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.

dsyme commented 2 days ago

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 commented 2 days ago •

@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)

DOCUMENTATION.md

slang25 commented 2 days ago

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

dustinmoris commented 2 days ago

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

dsyme commented 2 days ago •

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?)

dsyme commented 2 days ago

Yay! Thanks for finding that!