Task computation expression instead of async #53
Comments
|
@dustinmoris, I have done a fork of Giraffe and fully updated it to use task {} over async {} so we can test performance and usability, the project builds fine but when I swapped it into a web project previously using original async Giraffe, I did encounter one small issue, the overloading of Bind in task computation expression to accept both plain Task and Task<'T> is causing compiler inference issue when you let! bind Task<'T> values, forcing you to put a manual type inference statement Apart from the let! type inference hacking, applications run perfectly In the meantime, @TheAngryByrd, when you have a free moment, any chance you could test my TaskGiraffe fork and see if there is any benefit to this exercise? as mentioned before I think the benefit would really kick in when there is more complex, chained handlers where a lot of async Task calls are being chained. |
|
Yeah i'll make a run tonight and see if it gives any benefits. |
|
This may be related with aspnet/Mvc#5570 |
|
@TheAngryByrd Thanks! Hoping for just small improvement but should make a bigger difference once tested against larger handler pipelines. @gulshan Thanks for the link, strangely enough, this is unrelated to the compiler inference issue I'm having as this issue relates to using F# Async with Asp.Net Core and MVC/Middleware whereas my curiosity went down the line of, use Task directly (along with task computation expression) rather than Async at all. Async is generally the safer nicer construct to use with F# but given Asp.Net Core uses Task/Task<'T> throughout, along with all its related services, it seems to (potentially) make more sense to use a lightweight Task CE, where we trade bit of safety (cancellation tokens etc) for more speed and less Async to TASK calls and visa-verse. I can remove the overloaded Bind method on the TaskBuilder and force people to have to use a wrap function on Task so task {
do! runSimpleNonGenericTask // Task -> unit (Task overriding Bind)
let! v1 = foo.AsyncReadString() // Task<string> -> string
return v1
}...Its far more preferable. It does once again highlight though the issue of Task being painfully incompatible with composable functions that usually need generic type parameters passed if all methods using Task were Task would make life so much easier. |
|
@gerardtoconnor @TheAngryByrd afaik, Hopac is the one to go for perf. From my tests with just IO, F#'s Just to note, ASP.NET Core 2.0 will have native support for F#'s |
|
@imetallica thanks for the info, I have heard of Hopac and know its well known for performance but I didn't want to complicate the handlers with another construct, just use what asp.net uses natively ... but that being said, it appears that it can fully integrate with Task<'T> so might be worth redoing the bindings... I may wait and see if @dustinmoris and other contributors are open to Hopac before branching again. I originally was thinking F# Async would be faster also but reason I wanted to test this approach was the fact you wouldn't need to go between Async and Task all the time and the fact, due to every composable httphandler in the pipeline using async/task, most of the async tasks are actually CPU rather then IO, asp.net takes care of creating the httpContext & Request, so the only large async IO required is the writing into the response (and to lesser extent persistance/file) which will be done usually only at the very end of the pipeline. The Task may be slower on the IO part but in a pipeline of composable Async/Task Handlers, there may be better overall performance benefit ... but I am only speculating and curious when I noticed that I was constantly having to wrap CPU (non-IO) handler calculations in asyncs just so they would maintain the pipeline when they were in fact sync, not async. That's good to know on ASP.NET Core 2.0 ... makes most of the porting to Task work pointless as half the reason was to make async expressions easier ... and if performance turns out worse with Task, that's the final nail in the coffin. |
|
Please, don't introduce hopac as a core mandatory part of Giraffee - or at least make that an optional dependant package. Keep the barrier to entry as low as you can - especially with async support coming to ASP .NET in the future. |
|
@isaacabraham I tried rewrite with Hopac on dotnetcore and although built, couldn't use the reference Dll with Webapp ... it does feel like it's starting to overcomplicate things when the motivation to change from async is now much less with support coming in asp.net core 2.0. I think given my other concern was centered around unneeded CPU async needing async wrapper just to fit into the pipeline (for handlers that are not async eg routing), perhaps a better strategy is to have two handlers HttpHandlerVal & HttpHandlerAsync, and the compose (>=>) & bind (>==) operators could then be type static overloaded for multiple type combination cases (x4?) such that there is an additional wait bind on HttpHandlerAsync, before doing DU on option of HttpContext so that both produce the same piped HttpContext into the next handler in pipleline. Do you think this is again over complicating things, type inference should be able to figure chaining and should not complicate interface, only remove the need to wrap synchronous computation in async creating unneeded wrap/unwrap CPU aync? Non-refactored pseudo code type HttpHandlerAsync = HttpContext -> Async<HttpContext option>
with
static member (>=>) (a:HttpHandlerAsync) (b:HttpHandlerAsync) = fun ctx -> a ctx |> bindAsyncAsync b
static member (>=>) (a:HttpHandlerAsync) (b:HttpHandlerVal) = fun ctx -> a ctx |> bindAsyncVal b
and HttpHandlerVal = HttpContext -> HttpContext option
with
static member (>=>) (a:HttpHandlerVal) (b:HttpHandlerAsync) = fun ctx -> a ctx |> bindValAsync b
static member (>=>) (a:HttpHandlerVal) (b:HttpHandlerVal) = fun ctx -> a ctx |> bindValVal b**EDIT |
|
What would be a fair test? Just comparing these two? |
|
@TheAngryByrd that would be fine to compare IO impact of using Task. I would need to think up a more elaborate test with multiple routing and deeper handler chains for testing CPU/IO async tradeoff. With such a simple test it may come in the exact same or even slower but at least then I would know that it is a dead end. Thanks again for the assistance! |
|
Here are some results from OSX and Debian/Jessie docker image -https://gist.github.com/TheAngryByrd/67b7762517746c8be8119622c5ebed64 It does seem like task CE is pulling ahead ever so slightly. I also left in Can anyone else reproduce these results? This branch has the new code. https://github.com/TheAngryByrd/dotnet-web-benchmarks/tree/giraffe-task |
|
@TheAngryByrd that's great thanks, on the far more important linux/docker run it was on average 16.3% faster which is big, more then I thought it would to be honest. The 1.6% improvement on osx is not as important but at least still better. With it being only 4.7% behind MVC, with a far nicer interface, it makes Giraffe very compelling!! Out of interest are you able to tweak your harness to iterate through different request routes? it would just be a test between Giraffe(Async) and GiraffeTask I'm thinking, I would hope to see the outperformance increase as replicates real world app routing scenarios. I'm also looking into ways to remove the need to wrap sync operations in async/task that should hopefully improve further and bring inline with MVC but its tough as cannot compromise the simplicity of the interface. |
|
Yeah, should just be an extra parameter to |
|
@TheAngryByrd Thanks, I can prepare the route that will be same for both (type inference forces task / async to propagate down tree, as well as the plain list for you to work into metadata file. |
|
@isaacabraham the idea I'm suggesting is to do something like Freya and Suave did, with an extension to support Hopac (adding the Freya.Hopac or Suave.Hopac). I've discussed with Rodrigo Vidal on using Tries to do routing instead of pattern matching and it's faster, although he did the measurement, not me. Maybe asking him on Slack would we better. |
|
@imetallica funny you should mention trie routing, I have played around and it is faster, the issue is, you typically break the suave/giraffe piping structure if you are building a trie routing dictionary ... although an AST could be used to build a tire that both looked up routes and matched variables ... problem is you don't want to destroy the already established api too much. I can build trie routing if people are open to slight changes in piping ... if the routing was strictly splitting components by '/' you could use a radix tree instead but a fully flexible route tree needs to be a trie. |
|
|
I am messing around with a new structure that would remove the need to warp sync in async, and, through continuations, would more efficiently allow routing using a trie. Just wondering if people are open to change in suave interface, I know its so easy and nice but it can cause unneeded sync <> async bindings, my potential new format would chain/compose in the exact same fomat ie handler1 >=> handler2 but the handler functions would change such that type HttpHandler = HttpContext -> Async<HttpContext option>would change to type Continuation = HttpContext -> Task<HttpContext>
type HttpHandler = Continuation -> Continuation -> HttpContext -> Task<HttpContext>such that in the prior suave/giraffe format a handler would be let route path : HttpHandler =
fun ctx ->
if condition(path)
then Some ctx
else Nonewould then go to (in a potential new continuation format): let route path : HttpHandler =
fun succ fail ctx ->
if condition(path)
then succ ctx
else fail ctxI would rather not change the format if I could, but in a continuation format the handlers efficiently handle async in their context if needed and if sync, pass on the task from subsequent handlers, a push rather then pull format. I have started rewriting HttpHandlers in this format for performance testing, I'm hoping the performance will improve again. once this is done I can then start working on a trie router. The trie router would, in theory, take the composable handlers already in place and then, dependent on the number of child edges, match routes against path on a per char basis, with edges < 5 being seq iterative and >= 5 being on a binary search basis. I have already started design but want to rewrite and test continuation format before moving onto routing performance work. Would be great to gauge from the community if this work is wanted, for me suave has a nice structure and is so simple for users but given giraffe uses Asp.net Core for (mostly) performance purposes, I'm hoping we can tweak Giraffe to max performance such that it's better than MVC, in a far more logical functional format. |
|
@gerardtoconnor I would wait for the ASP.NET Core 2.0 to see if the change from The trie router I think should be a priority, if it doesn't depend on the changes you are proposing. |
|
@imetallica the change between Task and Async is simple and not an issue, task {} is 16.3% faster based on @TheAngryByrd linux Hardware benchmark but I want to improve overall usability as well as performance, not just change async vs Task. With Task being faster than async, ASP.Net Core 2.0 will improve the interface but not the performance (by that much). That being said I am focusing on the construct now, continuations over stack (pull) recursive trees. When I was thinking about the trie design, i realised the fastest way to move to the next edge/path in a route would be via direct continuation, not propagating a result back to a handler with state/node as nodes (like a linked list) usually would be one-directional stepping. I will work on Trei Routing as soon as I have finished re-writing of HttpHandlers in (succ->fail->ctx) format as i thing this is nicer and more consistent then wrapping sync HttpHandlers in 'Async.FromResult' functions that introduce unneeded async/task wrapping, In the (succ->fail->ctx) format, if a function is async, it computes the async wait in the function, otherwise it is a normal function that returnes the async computation of its succ/fail functions (succ = success / fail = failed route). |
|
@TheAngryByrd, using the following testApi (or similar) are you able to run a test on my just now updated GiraffeTask Fork (that uses continuations instead of binding) against regular Async Giraffe let testApi : HttpHandler =
choose [
GET >=>
choose [
route "/" >=> text "Hello world, from Giraffe!"
route "/test" >=> text "Giraffe test working"
subRoute "/auth" >=>
choose [
route "/dashboard" >=> text "Auth Dashboard"
route "/inbox" >=> text "Auth Inbox"
subRoute "/manager" >=>
route "/payroll" >=> text "Manager Payroll"
route "/timesheets" >=> text "Manager Timesheets"
]
route "/data" >=> text "json (weatherForecasts ())"
routef "/value/%s" >=> text
]
]The handler compose interface is the same, same HttpHandler functions (but routef might have slight different type sig as can now compose) So paths to iteratively hit on a reoccuing cycle: |
|
@gerardtoconnor With that route setup: Tested on debian/jessie with |
|
@TheAngryByrd thanks so much! managed to squeeze an additional 5%+ performance out on base root case while the performance boost on larger routes is now evident with 3 layer route being 37% faster. The average improvement across all routes being 26%.
I am now working on a new router system that uses Trie but it is messy, not only do you have to precompile the (multiple) Trie(s), rather than pure composed handler functions, but also somehow pass depth while not breaking the handler format ... this may take a bit of time, unfortunately. |
|
Hi, thanks for bringing this up and sorry for joining this party a bit late. I was with extremely poor internet over the last 5 days but seems like you guys made huge progress. I had a quick read through the thread and will digest it in more detail later today and discuss any next steps with you here. Thanks! |
Could you elaborate a bit more on the trade-off between safety vs. speed? @gerardtoconnor @imetallica @isaacabraham
Agreed. Personally I haven't used hopac yet and I would be interested to know how it outperforms Task/Async from the BCL? What is hopac doing differently or what is Task and Async doing more which hopac isn't doing so that it is faster? There must be some trade-offs as well I guess? I would want to support the easy extension of Giraffe so that anyone can use it with hopac through an additional NuGet package, but I would want to avoid too many competing/moving parts in the base library. If there is something that can be changed in Giraffe to easier support a hopac extension then let's discuss this in a separate thread. The native support for async has been logged in the MVC repository and not in the plain ASP.NET Core one. Are you sure that ASP.NET Core 2.0 will support F#'s async natively or will it only be for MVC action routes? @TheAngryByrd |
Full disclosure, I'm no expert now but afaik, Async is an interface construct like IEnumerable that prevents you having to deal with the underlying Task (like IEnumerator in IEnumerable), such that it sets up the cancellation tokens, propagates these tokens to children, while taking care of the generation and starting of the task, as well as its own cell/result trampolining structure. Async makes asynchronous really simple due to all these inbuilt checks & balances. In and around 2013 TPL was improved to enhance Task performance such that it is now faster than async in most situations. The purists will rightly state that I am cutting corners with not setting up cancel tokens etc but if you think about the use case in a performance web server,
All that being said ... like with Hopac, the extra performance won't be worth it if it scares people off so I may just develop on my fork until we come to a conclusion but I think the task {} CE is simple enough that users of async my not be too put off. @dustinmoris @imetallica @isaacabraham @dustinmoris |
|
@dustinmoris afaik, Hopac uses it's own threadpool and has somewhat good interop with Well, if |
|
@gerardtoconnor Thanks for the additional info on Task vs. Async. What you describe sounds reasonable to me and I am definitely open to make a change from async to task. I'll check out your branch now, but from your point of view what would be the remaining work to get your work merged? Regarding the alternative routing system I would have to see what the actual change would look like. Short answer is that I am generally open to any improvement, especially when it comes to performance, but I also want to make sure that ease of use and simplicity doesn't get neglected. Few questions:
The current routing system has the benefit that it is extremely flexible and easily composable. For example you could have a common library which implements shared handlers that can be easily plugged into any Giraffe web service. A dummy example would be: // Common handlers implemented in a shared library:
let notFound = setStatusCode 404 >=> text "Not Found"
let healthCheck = GET >=> route "/health" >=> text "I am healthy"
let commonAuthenticationHandler = ...
// Micro service
let webApp =
commonAuthenticationHandler >=> choose [
healthCheck
// other handlers/routes
notFound ]Would the alternative routing system allow the same easy of sharing code? These are just a few thoughts I have, but generally I am very interested in a better performing routing system. |
Agreed, I think both, a separated package which introduces a new namespace like |
|
@gerardtoconnor |
|
@dustinmoris yes all the Task CE is in The Task CE is just modified FSharpX, passed across with tweaks from @buybackoff, key is that Task has "Unwrap" method that can unwrap As mentioned, I've butchered In case you're wondering what point of messier continuation format is, it allows no unneeded async bindings (sync handlers have no async, they pass next functions task) and creates a near perfect task pipeline of minimal task generation ... I guess it may be academic but was fun messing around with and will hopefully lead to better more performant pipeline somehow. |
|
@dustinmoris @imetallica I have built a basic version of Trie router but need to tweak for performance etc. looks similar to usual format let webApi : HttpHandler =
GET >=>
routeTrie [
routeTf "/name%sssn%i" (fun (n,s) -> sprintf "your name is [%s] with social security number [%i]" n s |> text)
routeT "/" <| text "Hello world, from Giraffe!"
routeT "/test" <| text "Giraffe testing working"
subRouteT "/deep" ==>
routeTrie [
routeT "/hashtag" ==> text "hashtag deep"
routeTf "/map/%s" text
routeTf "/rev/%s/end" (sprintf "sring path hit:%s" >> text)
routeTf "/rev/%i/end" (fun v1 -> sprintf "integer path hit:%i" v1 |> text )
routeTf "/rev/%s/end/%s" (fun (v1,v2) -> sprintf "double match [%s] as well as [%s]" v1 v2 |> text)
]
routeT "/auth" ==> choose [
AuthTestHandler >=> text "your Authorised"
setStatusCode 404 >=> text "Not Found"
]
] the only break to the api would be that chained handler out of the route path function would be different I was thinking if we put regular route functions in a different namespace, with the Trie router also in a separate namespace so the dev makes conscience decision of which one they are using, they cannot both be used for routing. With Trie router |
|
@TheAngryByrd when you have a spare moment, would you be able to test performance once again of standard Giraffe against my "async-task" Giraffe branch? This is first implimentation of trie router. webapi Giraffe standard let chooseApi : HttpHandler =
choose [
route "/" >=> text "Hello world, from Giraffe!"
route "/test" >=> text "Giraffe test working"
route "/about" >=> text "Giraffe about page!"
route "/wheretofindus" >=> text "our location page"
route "/ourstory" >=> text "our story page"
route "/products" >=> text "product page"
route "/delivery" >=> text "delivery page"
routef "/data/%s/weather" (fun v -> sprintf "json (weatherForecasts (%s))" v |> text)
routef "/value/%s" text
subRoute "/auth" >=> choose [
route "/dashboard" >=> text "Auth Dashboard"
route "/inbox" >=> text "Auth Inbox"
route "/helpdesk" >=> text "Auth Helpdesk"
routef "/parse%slong%istrings%sand%sIntegers" (fun (a,b,c,d) -> sprintf "%s | %i | %s | %s" a b c d |> text)
routef "token/%s" (fun v -> text "following token recieved:" + v)
subRoute "/manager" >=> choose [
route "/payroll" >=> text "Manager Payroll"
route "/timesheets" >=> text "Manager Timesheets"
route "/teamview" >=> text "Manager Teamview"
routef "/team%ssales%f" (fun (t,s) -> sprintf "team %s had sales of %f" t s |> text)
routef "/accesscode/%i" (fun i -> sprintf "manager access close is %i" i |> text)
subRoute "/executive" >=> choose [
route "/finance" >=> text "executive finance"
route "/operations" >=> text "executive operations"
route "/mis" >=> text "executive mis"
routef "/area/%s" (sprintf "executive area %s" >> text)
routef "/area/%s/district/%s/costcode%i" (fun (a,d,c) -> sprintf "executive area %s district %s costcode %s" a d c |> text)
]
]
]
]webapi 'task-async' branch: let trieApi : HttpHandler =
routeTrie [
routeT "/" ==> text "Hello world, from Giraffe!"
routeT "/test" ==> text "Giraffe test working"
routeT "/about" ==> text "Giraffe about page!"
routeT "/wheretofindus" ==> text "our location page"
routeT "/ourstory" ==> text "our story page"
routeT "/products" ==> text "product page"
routeT "/delivery" ==> text "delivery page"
routeTf "/data/%s/weather" (fun v -> sprintf "json (weatherForecasts (%s))" v |> text)
routeTf "/value/%s" text
subRouteT "/auth" ==> routeTrie [
routeT "/dashboard" ==> text "Auth Dashboard"
routeT "/inbox" ==> text "Auth Inbox"
routeT "/helpdesk" ==> text "Auth Helpdesk"
routeTf "/parse%slong%istrings%sand%sIntegers" (fun (a,b,c,d) -> sprintf "%s | %i | %s | %s" a b c d |> text)
routeTf "token/%s" (fun v -> text "following token recieved:" + v)
subRouteT "/manager" ==> routeTrie [
routeT "/payroll" ==> text "Manager Payroll"
routeT "/timesheets" ==> text "Manager Timesheets"
routeT "/teamview" ==> text "Manager Teamview"
routeTf "/team%ssales%f" (fun (t,s) -> sprintf "team %s had sales of %f" t s |> text)
routeTf "/accesscode/%i" (fun i -> sprintf "manager access close is %i" i |> text)
subRouteT "/executive" ==> routeTrie [
routeT "/finance" ==> text "executive finance"
routeT "/operations" ==> text "executive operations"
routeT "/mis" ==> text "executive mis"
routeTf "/area/%s" (sprintf "executive area %s" >> text)
routeTf "/area/%s/district/%s/costcode%i" (fun (a,d,c) -> sprintf "executive area %s district %s costcode %s" a d c |> text)
]
]
]
]routes are: |
I vote for this. Better be explicit than implicit. Although I believe this should turn into moving the current routing functions to their own namespace, right? |
|
Ran it on OSX, don't use these result yet. Running on docker/debian overnight.
|
|
@TheAngryByrd Thanks so much, no need to run again on docker/debian, this initial run was actually really helpful and highlights some issues/fixes needed, so best to hold off on another run till these addressed. I have realised that we have gone off topic here given its thread for Task CE, I will open a new issue/feature request that specifically focuses on the router & router performance, we can leave this issue open till task CE merged. @TheAngryByrd Summary results on asp.netcore1.1 below, many thanks again.
|
|
Nice work guys. Just to summarise my view on both issues:
Thanks for the great work! |
|
Ok for more accurate numbers, https://gist.github.com/TheAngryByrd/b48dc2d9bba5db286693b082a9b9458c#file-debian-giraffe-vs-giraffetask-vs-giraffetasktrie-routes-csv trends look about the same upon glance |
|
@gerardtoconnor For the task CE, I recommend overloading the Example of doing it is Hopac https://github.com/Hopac/Hopac/blob/master/Libs/Hopac/Hopac.fs#L2235 |
|
@TheAngryByrd Completely agree and I have already implemented Bind Overloads at home in line with @imetallica comments. I will make PR in next day or so on this. |
|
Agree with @TheAngryByrd too. |
|
@TheAngryByrd @dustinmoris @imetallica sorry to complicate again but the current bind system, as with Suave, is bugging me, wrapping a sync result in a task/async just so it will fit in the API smells, and I know most don't mind/care but across many handlers, on thousands of requests, it adds up. In order to remove task/wraps I wrote the continuation format, instead of returning a Task of HttpContext option, you are provided two functions to run, succ / fail to choose the next path, the task from subsequent handlers being passed right through on stack efficiently. following gist is a subset that shows the difference, no bind function needed This continuation format adds 5%-15% additional performance over bind (depending on pipeline depth). On a review of the IL instructions, in normal Giraffe, both bind and compose have about 28 instructions each, 56 in total for full composition processing, my compose handler, due to the partially applied functions self-elimination/evaluate directly on the stack, it does everything in only 14 instructions 25%, a quarter the instructions. Instruction count is not exactly correlated to performance, size obj, heap/stack, instr type etc all factor in too, but it is usually a decent indication of performance, for a loose comparable estimate, less instructions the better. Is this format open to discussion? In the event everyone is uneasy with a continuation format, and its a no go, I have a backup plan that is not as performant/elegant but (should) come reasonably close, the return types would be ValueTask<'T> rather than Task<'T>, I have already built the computation expression for ValueTask<'T> and is working fine in my async-task branch (but performance and allocations need to be measured). ValueTask<'T> is a task hybrid return value that is a struct and doesn't need to allocate memory for wrapped sync result of Task<'T>. Internally it is just a struct that extends a Task<_> to include a result cell to be used if sync op but as its struct on stack with no GC, it is performant vs current task wrap scenario. The ValueTask<'T> was developed by asp.net core team as part of 'Channels' in the corefx module and I've added base ValueTask<'T> through nuget package of "System.Threading.Task.Extensions" to build full task CE of ValueTask<'T>, CE is overloaded with Task, Task<'T>, Async<'T> and ValueTask<'T> input to ValueTask<'T>. Please share your thoughts and let me know if I am over-cooking the performance pipeline. |
|
I would prefer to keep the API rather simple as I don't know if a lot of nested http handlers is even a common thing where this change would bring a significant performance boost? What level of nesting is required to reliably say it will improve performance? Another test I would like to run would be with a route which returns a razor view and perhaps even does a read from another data source before returning a response. I want to make sure that the task CE still performs better or at least close to async with the additional IO work that would be common for a real web app. |
|
@dustinmoris Ok, np, the simple bind format is composing handlers together also, nesting as you say, the difference being, using continuation format, you move directly to next function (with min Task Wrapping) while with bind, you back out your result, test, and then run next function or propagate a None (fail) back down the pipe-line stack. The execution of continuations is somewhat analogous to head vs tail recursion. tail recursion is ALWAYS better because it passes state/result forward, meaning no recursive chain on the stack (a stateful loop vs deep call stack). Anyway, enough rambling, I will implement ValueTask<'T> CE today and submit PR as this is a simple update that maintains API with no change. I will open a new issue to discuss HttpHandler performance formats and that can be an academic long-term architecture discussion weighing up perf/api format benefits. On the tests @TheAngryByrd so kindly performed, was 5% benefit on short stub routes, on deeper roots 10%-15% (it adds performance on every handler/route and this benefit should be predictably linear). comparison With ValueTask<'T> CE, everything will work the same as current async/Task only in the background the CE is wrapping 'T, Task, Async & ValueTask into ValueTask<'T>, when we return a sync value (ctx) it uses a struct return cell (of obj ref) rather than creating and wrapping a task around it, no heap alloc, no GC pressure. Let me know if this is what you want (overloaded task CE using ValueTask) and I'll submit PR. // {Async} HttpContext -> ValueTask<HttpContext Option>
fun ctx ->
task {
let! result = Task<bool>.GetAsync() // ValueTask holds Task<'U>
if result then
return Some ctx
else
return None
}
// {Sync 1} HttpContext -> ValueTask<HttpContext Option>
fun ctx ->
task {
if ctx.Request.path = "xyz"
then Some ctx // CE injects Sync Value into struct result cell avoiding Task wrap (as no preceeding continuarion bind of Task)
else None
}
// {Sync 2} HttpContext -> ValueTask<HttpContext Option> (alternative with no CE wrap using ValueTask Contructor)
fun ctx ->
if ctx.Request.path = "xyz"
then ValueTask<_>(Some ctx)
else ValueTask<_>(None)
} |
|
Hi, Today during my lunch break I had a quick start at resolving some of the merging conflicts in #59 and I had some thoughts on what would be the next steps after that. I was thinking we should run the following tests before setting anything in stone:
Each branch will produce a NuGet artifact which hopefully @TheAngryByrd can download and run the load tests for us :). By comparing the results we will see what would be the next reasonable step. That would also show us if ASP.NET Core 2.0 has better native support for Async and if that has an impact on perf. /cc @imetallica *) One of the main points discussed here was the unnecessary wrapping of Given that we suspect this to have a perf. impact we should test something like this: type ValueAsync<'T> =
| Value of 'T
| Async of Async<'T>
type HttpHandlerResult = ValueAsync<HttpContext option>
type HttpHandler = HttpContext -> HttpHandlerResult
let bindSync (handler : HttpHandler) =
fun (ctxOpt : HttpContext option) ->
match ctxOpt with
| None -> None |> Value
| Some ctx ->
match ctx.Response.HasStarted with
| true -> ctx |> Some |> Value
| false -> handler ctx
let bindAsync (handler : HttpHandler) =
fun (asyncCtxOpt : Async<HttpContext option>) ->
async {
let! ctxOpt = asyncCtxOpt
match ctxOpt with
| None -> return None
| Some ctx ->
match ctx.Response.HasStarted with
| true -> return Some ctx
| false ->
match handler ctx with
| Value ctxOpt2 -> return ctxOpt2
| Async asyncCtxOpt2 -> return! asyncCtxOpt2
} |> Async
let bind (handler : HttpHandler) =
fun (result : HttpHandlerResult) ->
match result with
| Value ctxOpt -> bindSync handler ctxOpt
| Async asyncCtxOpt -> bindAsync handler asyncCtxOptWhat are your thoughts on this plan? |
|
Quite a few points to cover :)
This is easy enough, was working fine before other PR request so move provides branch to test
task CE on asp.net 1.0.4 vs Async on core 2.0 is a bit of an apple and oranges test because there have been MASSIVE performance improvements across the whole framework so we would need to test both on 2.0. The Async interop is a built-in convenience overload that still requires tpl <> async boundary crossing, destroying performance, as well as then relying on slower cpu bound async between handlers (for IO should be comparable). The creation and destuction of the intermedite types logically is going to create unneeded fat all the way. It depends what we're trying to achive really, performance or path of least resistance ... i think of Giraffe as a functional asp.net core, which is max performance in nicest api possible (but I am admittidly over performance focused at times).
great minds think alike ... your ValueAsync implimentation is quite similar to one of the very first things I built testing Giraffe and messing around with api but I realised that the need to use differnt functions for each progression of sync/async was so messy, no-one would want this, only deter usage. I then tried near EVERYTHING to overload the operator to handle both but then i needed to create specialised static member FSharpFunc operator overloads and just wasnt coming together as all that magic is hidden away in source libraries. So although you are on the right track with ValueAsync, I think too ugly to impliment and if Im honest the even better work around I progressed to after, that is even more performant, was the continuation format as it doesnt need to bind and has 4x less IL instructions for same work!? these are two+ reasons not to create Tasks for sync work, your creating objects, adding obj ref call depth, and adding to task scheduler work ... all because we're being lazy :). we're able to throughput 100k routed resquests (not plain text) a second, every bit of performance matters so eliminating all unncessary object creations, refernce calls, and boxings are important. Asp.net core itself is using continuations for performance! I found blog subsequently explaining ... the Gist of continuation format again as reminder (means keep nice >=> format) ValueTask was an attempt by the ASP.NET Core guys to address this issue and is more performant then using DU as you have done with ValueAsync as is struct with ref cells to task & a value type such that both can be put in and calling the result is sync in case of result being set ... ValueTask was messy trying to set up in CE due to type inference not being able to filter by There is a balance we're trying to strike through I fully admit, its not a c/c++ ugly horrible webserver we're building but an elegant one where user loves to use it due to simple api, and we do all the difficult optimisations in the background hidden from user.... just like the router im building, it looks like normal handlers but its sucking everything in and compiling into a performant tree structure that smashes the normal routing upto +200% ... so maybe just do branches of each alternative, compare performance of each, and see how much we'll sacrifice in elegance for performance? |
dustinmoris
added
api change
|
This can be closed as submitted in PR #75 |
|
Cool, I will close it as soon as I have merged your PR. It looks good and I ran some load tests today as well which show a 9% improvement. |
I know in general, from a development standpoint, using the async {} with F# is far easier but I have been playing around with this great project, and noticed that I am constantly having to Async.AwaitTask as everything async in asp.net core is done in Tasks not F# Async and got me wondering, would the task {} computation expression me better in this library over async so that all the parts interfaced with asp.net core without the overhead of converting back and forth between async all the time?
task {} computation expression is already part of FSharpX and further, it has been modified and performance improved by @buybackoff as he explains on StackOverflow, with his updated version on Github FSharpAsyncVsTPL
such that performance is much improved over async {}, especially for internal (non IO/Http) which is the bulk of the async work in the chained handlers.
I know it would take a bit of re-writing such that HttpHandler would be (HttpContext -> Task<'HttpContext option'>, as well as updating CE from async {} to task {}, but given the platform is focusing on performance, using asp.net core, it seems a shame to introduce fat that is less compatible with the server (and its extended services) and the Task CE is already built to handle the same use case ... just food for thought, I can help in re-write if everyone thought it made sense, luckily the project is young enough that such a breaking change would not cause too much havoc?
The text was updated successfully, but these errors were encountered: