search

Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps
https://zaid-ajaj.github.io/Fable.Remoting/
MIT License
272 stars 54 forks source link

Support tasks #293

Closed kerams closed 1 year ago

kerams commented 2 years ago

Implements #134.

@isaacabraham, what's the goal here? Did you just want to avoid mucking around with Task <-> Async conversions in your code? This PR delivers on that front (though the directives I mentioned in https://github.com/Zaid-Ajaj/Fable.Remoting/issues/134#issuecomment-1037897673 are required now instead), but the conversions are still there, just pushed further down the stack, so no performance is gained. To eliminate them completely, we'd first have to reshuffle Paket groups and TFMs, because the relevant Remoting projects either don't target net6.0 or are pinned to FSharp.Core 4.7.2 owing to the lowest_matching constraint.

kerams commented 2 years ago

Ping @isaacabraham. Is this sufficient, do you think?

Zaid-Ajaj commented 2 years ago

Hi @kerams I understand this adds support for tasks on the server side of things but given that Fable does not support it, i think it would make sense to enable it on the dotnet client too since that is the only place where it will be usable (I saw the type alias trick but I wouldn't recommend using it just to avoid an explicit Async.AwaitTask call)

kerams commented 2 years ago

I can try, but it looks like it will involve a lot of (almost) duplicated code and many Task-Async to and fros, partly because of what I outlined above (we'd need to require .NET 6 and FSharp.Core 6 internally).

isaacabraham commented 2 years ago

Hi. Sorry for my lack of response here - been a bit snowed under recently.

My original thought was (and perhaps this is naive to think of it) was that, at least on the server, developers could just use native Tasks. It's (in my opinion) going to rapidly overtake async for most standard web programming work given its pervasive nature in .NET.

It appears (and I might be wrong here) that there are a few options:

  1. Developers program against Task on the server but at the end of the chain, the Task is implicitly converted / wrapped in an async. Nothing changes on the client; there are no real perf gains because we're still using async at the end of the day (in fact, could the extra wrapping cause perf losses?) but developers have the illusion of working in a purely task-based world.
  2. Developers program against Task on the server, and the Fable Remoting API supports Task. On the client, there could be some aliased type that can spoof as a Task but reuse Async (or whatever that does behind the scenes?) so you immediately come back to the "standard" world of Async.
  3. Some brand new implementation of Task CE in the browser so that we can use it there. But this feels like a lot of work to me.
kerams commented 2 years ago

I think my current proposed solution is a combination of 1 and 2. On the server side, the Remoting API would support Task returning functions, but internally there are still many unnecessary conversions, because I wanted to avoid duplicating too much library code and, more importantly, Remoting does not target a high enough version of FSharp.Core and TFMs. I'm sure this could be solved with a project, TFM and Paket group overhaul, but frankly I don't know where to start with that. Perhaps the simplest way to proceed would be to do what Giraffe does? Release a new major version and make a clean break - from now on Remoting users on the server will need FSharp.Core 6+ and to target net6.0+. Whoever has different requirements can still use previous versions. After all, the library has been stable for quite some time (new bugs can always surface, but I am not aware of any feature requests or new things we could implement). Not sure what Zaid would think about that though. An alternative would be to maintain parallel packages/package versions (as is the case with the Fable.Remoting.GiraffeNET5 project), but that just seems to be a maintenance nightmare...

Without extra support for tasks in Fable, or coming up with some other clever way of sharing the API contract instead of a record with fields returning asyncs/tasks (a major breaking change of itself), option 3 is out of the question. We thus have to resort to type spoofing I mentioned earlier (and that you'd unfortunately have to do yourself)

#if !TASK_AS_ASYNC && !FABLE_COMPILER
type Async<'a> = System.Threading.Tasks.Task<'a>
#endif

The client Fable projects sees asyncs, so business as usual, but server developers can return native tasks to Remoting.

I'd love to hear ideas about something better, but as things stand, this might be the best we can do.

Thorium commented 1 year ago

I think renaming a Task to Async is a weak workaround. They are different animals, and in a larger ecosystem there is need for both.

I share the view of @isaacabraham here: Typical webserver role is to fetch a resource from task-based asynchronous resource like "give me an item from database" or "read me a file". That's not an asynchronous workflow.

If you have 300 users online, it really starts to make a performance difference. Besides that, tasks have better stack-trace. They are fairly new constructs in F# and they are definitely not problem free yet.

First thing is maintainability and user experience, second thing is performance. That's why this path of "let's do task support and then later remove explicit conversions" is a good one.

Thorium commented 1 year ago

I tried this branch. Observations:

Server side works well:

Client side I had issues:

kerams commented 1 year ago

@Thorium, the type spoofing only happens in the context of Fable (because, as you noted, it has no support for tasks - you'd have to open a separate issue for Fable), so it has no impact on server performance. Also, I assume the client doesn't work (based on your second comment) because you omitted the conditional compilation.

kerams commented 1 year ago

Typical webserver role is to fetch a resource from task-based asynchronous resource like "give me an item from database" or "read me a file". That's not an asynchronous workflow.

I don't get what you mean. Async and Task are conceptually the same thing, with slightly different technicalities (hot-cold, repeatability, performance).

If you have 300 users online, it really starts to make a performance difference. Besides that, tasks have better stack-trace.

I haven't done any Task-Async benchmarks, but I can't imagine it would make a huge difference even when you have 300 users on your website concurrently. As noted here https://github.com/Zaid-Ajaj/Fable.Remoting/pull/293#issuecomment-1065863258, this PR only adds additional conversions when you use Task in your API, so the performance will actually be ever so slightly worse. I would like to remove all traces of Async from Remoting's internals, but it would involve bumping the FSharp.Core requirements (and maybe the minimum TFM to net6.0? I am not sure if it is required for task CEs).

Thorium commented 1 year ago

Task is CLR optimized asynchronous small task.

Async is non-CLR-optimized old way to do complex asynchronous workflows. It works better on more complex scenarios, but it's poor on performance.

Library like Fable remoting is not all about asynchronous workflows, so it should migrate towards the new way as soon as possible.

If Fable is not going to support easy usage of task, meanwhile all the other .NET (C# and now F#) libraries are taking the Task direction, will Fable's fate be exactly what happened to Suave: People try to find ways to migrate away from it. When the Suave maintainers finally understood, it was already too late.

Edit: Sorry about the rant.

kerams commented 1 year ago

https://github.com/Zaid-Ajaj/Fable.Remoting/pull/323

kerams commented 1 year ago

@Thorium, if it's performance you're after, I suggest you don't upgrade just yet. On my machine a single call is about 0.5ms slower than before (and this is tasks only, from top to bottom), possibly because of https://github.com/dotnet/fsharp/issues/12839#issuecomment-1264370200.