Task computation expression instead of async

[gerardtoconnor]

Task computation expression instead of async

[TheAngryByrd]

Yeah i'll make a run tonight and see if it gives any benefits.

[isaacabraham]

@imetallica

• Extension as e.g. nuget package - great idea.
• Core part of project - Not great IMHO.

[dustinmoris]

@gerardtoconnor

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.

Could you elaborate a bit more on the trade-off between safety vs. speed?

@gerardtoconnor @imetallica @isaacabraham

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

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.

@imetallica @gulshan

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
Thanks for running all the perf tests!

[gerardtoconnor]

@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 ==> . <| or bracket wrap () instead of >=> for just the initial route map operator. Is this too much of a pain?

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 choose is still used for non-routing fallback like authorisation etc, all composable like before.

[isaacabraham]

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.

[gerardtoconnor]

@dustinmoris

I'll check out your branch now, but from your point of view what would be the remaining work to get your work merged?

Well ... I kind of whet off on a tangent on my branch and changed the entire binding system to continuations to see if would be faster again (which it was), forgetting to branch before I did ... I can quickly copy over task stuff though once finished router work.

If the change to task is dealt as a separate issue, what would be the additional performance gain from the new routing system?

Not finished yet but I am confident it will be faster, the benefit grows the more route complexity in the application ... don't wanna look foolish too so let's wait and see!

The trei router can of course be an alternative to "choose" & "route", I'm hoping it can be swapped in but the type signatures would be very different in that:

let route (path:string) (fn:HttpHandler) : Node -> Node = <code> // where fn is standard tree of composed functions
let routef (path:StringFormat<_,'T>) (fn:'T -> HttpHandler) : Node -> Node = <code>

let router (routeState:'U?) (routeList: (Node -> Node) list ) : HttpHandler = <code> 

As the router is compiling a trie node structure using composed HttpHandler functions (such that should look exactly the same) the only difference is router needs to receive a router state ( esp for nested routes ). It is a similar issue to what you had with subRoute but for performance, I'd rather not write to ctx and figure out a better way if I can.
Each route function generates a node mapping function that then, one by one is run through the routers root node to build the trie. My implementation is using custom variable parsers, no regex, to work with cursor format of search.

I fully understand and appreciate the need for the httphandlers to remain composable in same/similar style.

[dustinmoris]

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.

[gerardtoconnor]

@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
let! (v:'T) = Task<'T>
which is not ideal. I may reach out to some of the geniuses in the community and see if there is a way to get the type inference working properly without any messy workarounds.

Apart from the let! type inference hacking, applications run perfectly
My Task Fork

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.

[gulshan]

This may be related with aspnet/Mvc#5570

[gerardtoconnor]

@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 goes to Task<unit>, like what Async did with Async.StartAsTask ... but given the type signature should allow the correct inference, and it is much nicer to be able to write:

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.

[imetallica]

@gerardtoconnor @TheAngryByrd afaik, Hopac is the one to go for perf.

From my tests with just IO, F#'s Async is way faster than C#'s Task. Since 99% of the time you are doing IO with web requests/responses, I don't see any reason to migrate to Task workflows.

Just to note, ASP.NET Core 2.0 will have native support for F#'s Async, so you will not need to do this conversion everywhere.

[gerardtoconnor]

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

[gerardtoconnor]

@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
Never mind on the overloaded operators ... compiler throws an error that overloaded operators are not allowed type alias ... looks like there is no easy solution that maintains simple pipe api/interface.

[TheAngryByrd]

What would be a fair test? Just comparing these two?

let handleAsync (ctx) = async {
    return! text "Hello World!" ctx
}

let webApp = 
    choose [
        GET  >=> route "/" >=> handleAsync
    ]

let handleAsync (ctx) = task {
    return! text "Hello World!" ctx
}
let webApp = 
    choose [
        GET  >=> route "/" >=> handleAsync
    ]

[gerardtoconnor]

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

[TheAngryByrd]

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 Kestrel/Plain and Kestrel/MVC for reference. I would also tend to trust the docker/debian results more since no one in their right mind would deploy to osx.

Can anyone else reproduce these results? This branch has the new code.

https://github.com/TheAngryByrd/dotnet-web-benchmarks/tree/giraffe-task

[gerardtoconnor]

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

[TheAngryByrd]

Yeah, should just be an extra parameter to runBenchmark function. Either we could have a list of routes each project should implement and/or have a metadata file with the list of routes it wants tested.

[gerardtoconnor]

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

[imetallica]

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

[gerardtoconnor]

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

[gerardtoconnor]

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 None

would 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 ctx

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

[imetallica]

@gerardtoconnor I would wait for the ASP.NET Core 2.0 to see if the change from Async to Task is justified, in terms of performance and/or convenience, because as I stated before, it seems it will support asynchronous workflows natively.

The trie router I think should be a priority, if it doesn't depend on the changes you are proposing.

[gerardtoconnor]

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

[gerardtoconnor]

@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:
/
/test
/auth/dashboard
/auth/inbox
/auth/manager/payroll
/auth/manager/timesheets
/data
/value/testgetvaluesubmission

[TheAngryByrd]

@gerardtoconnor With that route setup:
https://gist.github.com/TheAngryByrd/b48dc2d9bba5db286693b082a9b9458c

Tested on debian/jessie with netcoreapp1.1 and net462 (mono 4.6.2.16). GiraffeTask still seems to win on both.

[gerardtoconnor]

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

route	Giraffe	GiraffeTask	Improvement
/	52,697	63,625	20.74%
/auth/dashboard	46,814	58,376	24.70%
/auth/inbox	44,763	57,317	28.05%
/auth/manager/payroll	36,616	50,235	37.19%
/auth/timesheets	38,612	50,613	31.08%
/data	50,019	60,476	20.91%
/test	50,525	63,409	25.50%
/value/testgetvaluesubmission	37,142	45,177	21.63%
Total Averages	44,648	56,153	25.77%

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.

[dustinmoris]

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!

[gerardtoconnor]

@dustinmoris

Could you elaborate a bit more on the trade-off between safety vs. speed?

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,

• Web Server is like one big function that takes a request and must return a response, so in this thinking, at the risk of firing off unneeded work, given we need the task/work to finish, and return the response, it's an acceptable trade-off to ensure we get a predictable response.
• Any child/Parallel task that needs a cancellation can be manually implemented.
• Async/Task cancellation is usually done with UI/Parallel tasks such that a long-running computation can be canceled or, with a UI, a user can communicate to a background task to stop it etc. Most use cases for web server will be async series calculations rather than parallel.
• Given the project is built on Asp.Net Core vs eg Suave etc, I presume performance married to an elegant api is the goal, given we can test, tweak and ensure Task is being used properly, it makes more sense to use it if we can deliver much better performance while maintaining ease of use.

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
I think we are all in agreement to put a pin on Hopac support for now, and not as part of main library either way.

@dustinmoris
Are you open to an alterate routing system based on a precompiled route Trie structure? it may change the handler interface a bit which is the only issue. I may build it, test it and if performance it alot better ... we can then worry about handler construct i guess.

[imetallica]

@dustinmoris afaik, Hopac uses it's own threadpool and has somewhat good interop with Async and Task. Maybe we could have a separated namespace and package just for Hopac, like Giraffe.Hopac or something. Haven't tried it myself, but from the comparisons from Suave and Freya, Hopac is quite fast.

Well, if Task is faster, then it's the way to go forward. @gerardtoconnor I haven't had time to try out your fork, but can you give a headlight on how would it look like mixing Async and Task stuff inside Task CE? Also, just a side question: can you overload let! inside the CE to work with both Async and Task?

[dustinmoris]

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

• If the change to task is dealt as a separate issue, what would be the additional performance gain from the new routing system?
• Could the alternative routing system not be implemented as an additional alternative instead of a replacement?

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.

[dustinmoris]

@imetallica

Maybe we could have a separated namespace and package just for Hopac, like Giraffe.Hopac or something. Haven't tried it myself, but from the comparisons from Suave and Freya, Hopac is quite fast.

Agreed, I think both, a separated package which introduces a new namespace like Giraffe.Hopac would be my preferred choice.

[gerardtoconnor]

@imetallica I have overloaded Binder before and worked fine taking Async as well as Task, I'm pretty sure Hopac does it also, in fact the task {} in my fork is already using binding overload for Task/Task (although type inference gets a bit messed up with plain Task)

[dustinmoris]

@gerardtoconnor
From what I could see the work for the alternative routing system was only two commits after you finished the change to task, so it was fairly easy to review the async-task work in isolation. I think it looks good, I wanted to spend a bit more time looking over the AsyncTask.fs file which is essentially the bulk of the work, but otherwise I think it looks good.

[gerardtoconnor]

@dustinmoris yes all the Task CE is in AsyncTask.fs ( the reason I did not put it in obvious Computation Expression.fs file was due to the preceding Common.fs having a dependent Async/Task expression in it.

The Task CE is just modified FSharpX, passed across with tweaks from @buybackoff, key is that Task has "Unwrap" method that can unwrap Task<Task<'T>> -> Task<'T> so that continuations directly apply without touching the scheduler (or so I believe!?), meaning minimises unneeded wrapping Task constructs.

As mentioned, I've butchered HttpHandlers.fs with continuation HttpHandler format, so avoid that, and I have kept the Router work in HttpRouter.fs to keep away from nice clean code elsewhere. It's still a messy WIP so please don't garnish anything from router code yet.

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.

[gerardtoconnor]

@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:
/
/test
/about
/wheretofindus
/ourstory
/products
/delivery
/data/sunnydrycold/weather
/value/thisisavaluethatisbeingpassed
/auth/dashboard
/auth/inbox
/auth/helpdesk
/auth/parsefirstlong987654321stringssecondandthirdIntegers
/authtoken/as8d7f098adsf897asdf7a09dfasd7f9as7df987
/auth/manager/payroll
/auth/manager/timesheets
/auth/manager/teamview
/auth/manager/teambravosales5345545.34544
/auth/manager/accesscode/57646878
/auth/manager/executive/finance
/auth/manager/executive/operations
/auth/manager/executive/mis
/auth/manager/executive/area/globaloperationcentral
/auth/manager/executive/area/london/district/east/costcode8087

[imetallica]

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 choose is still used for non-routing fallback like authorisation etc, all composable like before.

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?

[TheAngryByrd]

Ran it on OSX, don't use these result yet. Running on docker/debian overnight.

Provided Giraffe vs Giraffe with Task vs Giraffe with Task and Trie in csv and md
https://gist.github.com/TheAngryByrd/b48dc2d9bba5db286693b082a9b9458c#file-giraffe-vs-giraffetask-vs-giraffetasktrie-routes-csv

[gerardtoconnor]

@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.
@dustinmoris @imetallica @TheAngryByrd
There is a bug in one of the routes
/auth/manager/executive/area/london/district/east/costcode8087
as is +50% slower than both Giraffe & GiraffeTask, with all other routes being faster or same.
The small short routes are coming in same as GiraffeTask, but there are big speed improvements +20% on deep routes, while my custom parsers (non-regex) are creating speed improvements of up to 40%-60% (which is significant).

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.

URLS	Giraffe	GiraffeTask	GiraffeTaskTrie	Gir/Task	Gir/Trie	Task/Trie
/	2300198	2370840	2349487	3%	2%	-1%
/about	2303891	2391520	2324699	4%	1%	-3%
/auth/dashboard	2201030	2325389	2316402	6%	5%	0%
/auth/helpdesk	2194300	2322608	2303590	6%	5%	-1%
/auth/inbox	2207080	2317181	2320119	5%	5%	0%
/auth/manager/accesscode/57646878	1517963	1792291	2271399	18%	50%	27%
/auth/manager/executive/area/globaloperationcentral	1390840	1617520	2301895	16%	66%	42%
/auth/manager/executive/area/london/district/east/costcode8087	1371384	1561244	633487	14%	-54%	-59%
/auth/manager/executive/finance	1614182	1930551	2311994	20%	43%	20%
/auth/manager/executive/mis	1620931	1963573	2320318	21%	43%	18%
/auth/manager/executive/operations	1586678	1954403	2337297	23%	47%	20%
/auth/manager/payroll	1948867	2211415	2306816	13%	18%	4%
/auth/manager/teambravosales5345545.34544	1260250	1365431	2224991	8%	77%	63%
/auth/manager/teamview	1876877	2188016	2342409	17%	25%	7%
/auth/manager/timesheets	1899790	2199281	2316064	16%	22%	5%
/auth/parsefirstlong987654321stringssecondandthirdIntegers	1330782	1503244	2264492	13%	70%	51%
/authtoken/as8d7f098adsf897asdf7a09dfasd7f9as7df987	1718061	2088203	2287279	22%	33%	10%
/data/sunnydrycold/weather	2205647	2304337	2287102	4%	4%	-1%
/delivery	2292301	2322192	2343339	1%	2%	1%
/ourstory	2303892	2342938	2370148	2%	3%	1%
/products	2315674	2339945	2353792	1%	2%	1%
/test	2305624	2377866	2374676	3%	3%	0%
/value/thisisavaluethatisbeingpassed	2155155	2295704	2306295	7%	7%	0%
/wheretofindus	2303105	2332918	2368049	1%	3%	2%
Grand Total	46224502	50418610	53936139	10%	20%	9%

[dustinmoris]

Nice work guys. Just to summarise my view on both issues:

• I like the speed improvement going from Async to Task and the only thing we need to make sure is that we get the task {} workflow right. I'd love to see a concrete pull request for that piece of work so we can start reviewing it in more depth and hopefully finish it fairly soon.
• I also like the idea of a trie routing system alongside the existing routing system to give users a choice. Depending on how far you think you are with your prototype we could move this discussion and work into a pull request as well and take it from there...

Thanks for the great work!

[TheAngryByrd]

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

[TheAngryByrd]

@gerardtoconnor For the task CE, I recommend overloading the Bind and ReturnFrom functions on the TaskBuilder to accept Async<'a> as well to help cut down on the need for using Async.StartAsTask and help with transition. @dustinmoris do you agree with this?

Example of doing it is Hopac https://github.com/Hopac/Hopac/blob/master/Libs/Hopac/Hopac.fs#L2235

[gerardtoconnor]

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

[dustinmoris]

Agree with @TheAngryByrd too.

[gerardtoconnor]

@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
Continuation HttpHandler format

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.

[gerardtoconnor]

@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
links between TaskBind (Current) and TaskCont (Mine) confirm benefit.

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

[dustinmoris]

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:

1. Resolve all conflicts and get the new TaskCE working in a separate branch
2. Create a branch with normal Async handlers but run it on ASP.NET Core 2.0 (there's already preview packages)
3. Create another feature branch to test the performance of something like a ValueAsync<'T> handlers*
4. If ValueAsync<'T> outperforms normal Async handlers, then we should additionally create a branch with it on ASP.NET Core 2.0 and see if it gains even more perf or not

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

*)
Re ValueAsync<'T>

One of the main points discussed here was the unnecessary wrapping of HttpContext option in Async<_> when some handlers were only doing sync work.

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 asyncCtxOpt

What are your thoughts on this plan?
/cc @gerardtoconnor

[gerardtoconnor]

Quite a few points to cover :)

Resolve all conflicts and get the new TaskCE working in a separate branch

This is easy enough, was working fine before other PR request so move provides branch to test

Create a branch with normal Async handlers but run it on ASP.NET Core 2.0 (there's already preview packages)

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

Create another feature branch to test the performance of something like a ValueAsync<'T> handlers* If ValueAsync<'T> outperforms normal Async handlers, then we should additionally create a branch with it on ASP.NET Core 2.0 and see if it gains even more perf or not

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 next requestDelegate is a continuation just like my 'succ' fn, we need a fail fn only due to choose as a method of failing a test and moving to another alternative ... with router now using compiled tree, has no None/fail. (there may be a way to cleanly change to just fun next ctx -> signature, returning Task.FromResult ctx instead of fail ctx but I suppose not much of a difference.

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 GetAwaiter() member properly as mentioned in other thread on fs-lang-suggestions

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?

[gerardtoconnor]

This can be closed as submitted in PR #75

[dustinmoris]

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.