Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fsharp-giraffe #1

Merged
merged 10 commits into from Sep 24, 2020
Merged

fsharp-giraffe #1

merged 10 commits into from Sep 24, 2020

Conversation

rfvgyhn
Copy link
Contributor

@rfvgyhn rfvgyhn commented Sep 22, 2020

If you're considering F#, I'd highly recommend using Giraffe over Suave since performance is a concern. Giraffe runs on top of asp.net core/kestrel so it inherits all of its great performance.

Changes:

  • fsharp-giraffe: Basically a copy/paste of the sync suave project with suave swapped out for giraffe
  • fsharp-giraffe-async:
    • Async version of suave
    • Uses tasks instead of async so no conversions have to be done. F# had an async programming model before C# did. C# got async/await later on and has a different implementation via Tasks.
    • Checks if an Expr requires any async calls. If not (fizzbuzz), it runs synchronous method calls and avoids the overhead of Tasks. This was just a quick hack to get it working. There's a lot of code duplication and poor names.
  • Cleaned up the suave projects so they can run without modification (just a few small things like case sensitivity of project names).

I'm by no means an F# expert (lot's of .net experience though) so I can't really critique the F# itself. I just wanted to make sure you had a giraffe benchmark so you can make a more informed decision. If you do decide on F#, you might also consider using Saturn which is an opinionated web framework that runs on top of Giraffe.

I wasn't able to run the ocaml projects without modification. I didn't include those changes in the branch though. Dune wouldn't build on my machine (arch linux) so I had to use v2.7.0. I also needed to rename ocaml-sync.opam to ocaml-httpaf.opam. The exe symlink was missing in the ocaml-httpaf-lwt project as well.

Here are the results of each on my machine.

Benchmarking fsharp-giraffe
  Measuring fizzbuzz
    Reqs/s:       24962.95
    Avg:       4.52ms
    Errors:      N/A
    Slowest:   98.23ms
  Measuring fizzboom
    Reqs/s:       1.00
    Avg:       6.06s
    Errors:      N/A
    Slowest:   6.07s
Benchmarking fsharp-giraffe-async
  Measuring fizzbuzz
    Reqs/s:       19476.78
    Avg:       5.34ms
    Errors:      N/A
    Slowest:   66.98ms
  Measuring fizzboom
    Reqs/s:       8.99
    Avg:       1.03s
    Errors:      N/A
    Slowest:   1.13s
Benchmarking fsharp-suave
  Measuring fizzbuzz
    Reqs/s:       9733.18
    Avg:       10.53ms
    Errors:      N/A
    Slowest:   55.07ms
  Measuring fizzboom
    Reqs/s:       1.00
    Avg:       6.06s
    Errors:      N/A
    Slowest:   6.06s
Benchmarking fsharp-suave-async
  Measuring fizzbuzz
    Reqs/s:       1147.71
    Avg:       86.47ms
    Errors:      N/A
    Slowest:   233.68ms
  Measuring fizzboom
    Reqs/s:       1.00
    Avg:       6.09s
    Errors:      N/A
    Slowest:   6.09s
Skipping broken benchmark: fsharp-suave-partial-async
Benchmarking ocaml-httpaf
  Measuring fizzbuzz
    Reqs/s:       14034.62
    Avg:       7.10ms
    Errors:      N/A
    Slowest:   14.75ms
  Measuring fizzboom
    Reqs/s:       0.10
    Avg:       6.02s
    Errors:      N/A
    Slowest:   6.02s
Benchmarking ocaml-httpaf-lwt
  Measuring fizzbuzz
    Reqs/s:       14158.74
    Avg:       7.04ms
    Errors:      N/A
    Slowest:   14.37ms
  Measuring fizzboom
    Reqs/s:       0.10
    Avg:       6.01s
    Errors:      N/A
    Slowest:   6.01s
Benchmarking rust-hyper
  Measuring fizzbuzz
    Reqs/s:       15985.69
    Avg:       6.23ms
    Errors:      N/A
    Slowest:   27.01ms
  Invalid fizzboom output
Skipping broken benchmark: rust-hyper-async

@pbiggar
Copy link
Member

pbiggar commented Sep 22, 2020

This is awesome, thank you!

If you're considering F#, I'd highly recommend using Giraffe over Suave since performance is a concern. Giraffe runs on top of asp.net core/kestrel so it inherits all of its great performance.

Yes, I can see what a difference this makes. I just want to take the best performance from each language, so long as it doesn't mess with the programming model (as is, we don't have the write terrible code to get excellent performance). So I'd suggest we remove the suave benchmarks entirely.

Uses tasks instead of async so no conversions have to be done. F# had an async programming model before C# did. C# got async/await later on and has a different implementation via Tasks.

Great, this aligns nicely with the goals.

Checks if an Expr requires any async calls. If not (fizzbuzz), it runs synchronous method calls and avoids the overhead of Tasks. This was just a quick hack to get it working. There's a lot of code duplication and poor names.

This is an interesting optimization, similar to the partial async options. If I think of the programming model I want to support in Dark (which cause the "rules" of the benchmark), I think it would be better for a benchmark to only have a single eval function, not two (also if you look at the logic of the real dark eval function, it is much more complicated and so duplicating it would be risky).

I'd suggest breaking this into three benchmarks:

  • sync: not async/tasks/etc
  • async: simple async/task/etc
  • optimized async (see below)

By the optimized version, I mean having two stdlib function types, sync and async, like you have here. I'd be very interested to see if they have performance benefits, so having them as a separate benchmark vs the "plain" async would be super interesting.

Cleaned up the suave projects so they can run without modification (just a few small things like case sensitivity of project names).

Appreciate it! Though, I think best to remove them all the same.

I'm by no means an F# expert (lot's of .net experience though) so I can't really critique the F# itself. I just wanted to make sure you had a giraffe benchmark so you can make a more informed decision. If you do decide on F#, you might also consider using Saturn which is an opinionated web framework that runs on top of Giraffe.

Thanks. As a complete newbie in F#, this advice is appreciated!

I wasn't able to run the ocaml projects without modification. I didn't include those changes in the branch though. Dune wouldn't build on my machine (arch linux) so I had to use v2.7.0. I also needed to rename ocaml-sync.opam to ocaml-httpaf.opam. The exe symlink was missing in the ocaml-httpaf-lwt project as well.

I'll take a look at fixing these, thanks. Might add docker or CI now there are contributors (also, matches the deployment model).

@pbiggar
Copy link
Member

pbiggar commented Sep 22, 2020

Oh, one more thing to add. The important thing on the benchmark is not how high the fizzbuzz results go, but about making the fizzboom results go up. Right now they all suck - presumably because I'm not coding them right to allow other requests to be processed while the HttpClient::get call is running. I'm working on that in the OCaml versions at the moment - if you know how to enable it in the F# version that would be great!

@rfvgyhn
Copy link
Contributor Author

rfvgyhn commented Sep 24, 2020

I might be misunderstanding what you mean, but I believe the giraffe-async version already works that way. The current benchmark runs 10 connections with a minimum response time of one second making the ideal result 10 requests/sec, right? The giraffe-async is at 8.99 with an average time of 1.03s. Dropping the benchmark duration down to two seconds still results in more than 2 requests/sec meaning each request isn't waiting for the previous to finish the httpbin request.

Regarding the simple/optimized async versions, there is a significant performance penalty wrapping all the eval return values in tasks when there isn't any async work to be done. That's how I first tried it when I just ported over the suave code. I believe it resulted in ~9k requests/sec vs the ~19k in its current form on my machine. There may be a simple way around that by skipping the execution bubble with the unsafe versions of the computation expressions (uvtask, uply). I'm not too familiar with that though so I can't say for sure. It may also not matter depending on what Dark users generally use. How common/rare is it that user's expressions will only have non-IO bound code? If it's rare, it may be worth it to take the performance hit in order to keep the code easier to maintain (assuming uvtask and/or uply can't make up for it).

@pbiggar
Copy link
Member

pbiggar commented Sep 24, 2020

I might be misunderstanding what you mean, but I believe the giraffe-async version already works that way. The current benchmark runs 10 connections with a minimum response time of one second making the ideal result 10 requests/sec, right? The giraffe-async is at 8.99 with an average time of 1.03s. Dropping the benchmark duration down to two seconds still results in more than 2 requests/sec meaning each request isn't waiting for the previous to finish the httpbin request.

Yes, you're correct here. I've realized there's a problem in how I'm benchmarking. I'm working on improving that.

Regarding the simple/optimized async versions, there is a significant performance penalty wrapping all the eval return values in tasks when there isn't any async work to be done. That's how I first tried it when I just ported over the suave code. I believe it resulted in ~9k requests/sec vs the ~19k in its current form on my machine. There may be a simple way around that by skipping the execution bubble with the unsafe versions of the computation expressions (uvtask, uply). I'm not too familiar with that though so I can't say for sure. It may also not matter depending on what Dark users generally use. How common/rare is it that user's expressions will only have non-IO bound code? If it's rare, it may be worth it to take the performance hit in order to keep the code easier to maintain (assuming uvtask and/or uply can't make up for it).

I'm setting up the rules of the game such that maintenance is paramount. I can see arguments in the other direction but having only 1 eval definition is the rule I'm working with :) If it can be made work with a single eval and without greatly complicating the code, I think that's a fair implementation though.

I saw similar slowdowns with the suave version, and the fsharp-suave-partial-async was an attempt to workaround that which was surprisingly effective.

@pbiggar pbiggar merged commit 98b9a05 into darklang:main Sep 24, 2020
@pbiggar
Copy link
Member

pbiggar commented Sep 24, 2020

I hand merged this in and I can address the issues I raised. Thanks again for the great PR!

@pbiggar
Copy link
Member

pbiggar commented Sep 25, 2020

@rfvgyhn I have some questions about the code here. After benchmarking it, I realized that it didn't have the right semantics. Fizzboom should always take 6s minimum, but as you can see in the results, this has a much lower average runtime.

This is because it's running all the Fizzboom's requests in parallel, see:

let! args = Task.WhenAll(args)

let! result = Task.WhenAll(result)

The intended way is for them to block (and instead process other computation while blocking), see the OCaml version:

let* args = Lwt_list.map_s (eval env st) args in

Lwt_list.map_s (fun dv ->

Do you know how I'd do that with Tasks?

@rfvgyhn
Copy link
Contributor Author

rfvgyhn commented Sep 25, 2020

I completely overlooked that. I'm not sure how it works in OCaml, but one of the main differences between Task<T> and Async<T> is that tasks are hot and asyncs are cold. As far as I understand, once you have a collection of tasks, all of them are already started. As long as none of the tasks throw exceptions or are cancelled, the resulting collection will be in the same order as the initial collection of tasks though (see remarks section). If you need to run things sequentially for reasons beyond preserving order, you might have to switch back to async.

@pbiggar
Copy link
Member

pbiggar commented Sep 25, 2020

one of the main differences between Task<T> and Async<T> is that tasks are hot and asyncs are cold

I'm not sure what hot and cold mean here. Do you mean that the thread running a task does not move to other work during IO?

@pbiggar
Copy link
Member

pbiggar commented Sep 25, 2020

I'm not sure what hot and cold mean here. Do you mean that the thread running a task does not move to other work during IO?

nvm, found a definition, thanks

@rfvgyhn
Copy link
Contributor Author

rfvgyhn commented Sep 26, 2020

@pbiggar Just wanted to mention you might want to download RC1 of .net 5 (which is scheduled for full release in November) and run your benchmarks against that. Big performance gains for giraffe. https://nitter.net/ben_a_adams/status/1309139564099964936

@pbiggar
Copy link
Member

pbiggar commented Sep 27, 2020

@pbiggar Just wanted to mention you might want to download RC1 of .net 5 (which is scheduled for full release in November) and run your benchmarks against that. Big performance gains for giraffe. https://nitter.net/ben_a_adams/status/1309139564099964936

Thanks!

Also, fyi, I figured out how to do in-order execution with tasks: db50aa1

@pbiggar
Copy link
Member

pbiggar commented Sep 27, 2020

Edit: pasted wrong the first time

@pbiggar Just wanted to mention you might want to download RC1 of .net 5 (which is scheduled for full release in November) and run your benchmarks against that. Big performance gains for giraffe. https://nitter.net/ben_a_adams/status/1309139564099964936

Before:

  Measuring fizzbuzz
    Reqs/s:       12861.75
    Avg:          8.84ms
    Errors:       N/A
    Slowest:      119.27ms

After:

fsharp-giraffe-async/measure_fizzbuzz
    Reqs/s:       13209.20
    Avg:          9.68ms
    Errors:       N/A
    Slowest:      202.09ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants