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 #3859

Merged
merged 16 commits into from Jun 12, 2018

Conversation

Projects
None yet
6 participants
@dv00d00
Contributor

dv00d00 commented Jun 12, 2018

Added FSharp language with giraffe implementation of plaintext, json, and jsonutf8 endpoints

.UseKestrel()
.Configure(Action<IApplicationBuilder> configureApp)
.ConfigureServices(configureServices)
.ConfigureLogging(configureLogging)

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Don't configure logging
Rule 3.

All test implementations must disable all disk logging. For many reasons, we expect all tests will run without writing logs to disk. Most importantly, the volume of requests is sufficiently high to fill up disks even with only a single line written to disk per request. Please disable all forms of disk logging. We recommend but do not require disabling console logging as well.

Every other framework disables logging completely so having it enabled will be a disadvantage

// ---------------------------------
let configureApp (app : IApplicationBuilder) =
let env = app.ApplicationServices.GetService<IHostingEnvironment>()

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Remove both error handlers (for same reason as logging)

@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Don't need this file, is used when behind IIS

let jsonUtf8 (data:obj) : HttpHandler =
fun (next : HttpFunc) (ctx : HttpContext) ->
let bytes = Utf8Json.JsonSerializer.Serialize(data)
ctx.SetContentType "application/json"

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Additionally set the content length (bytes.Length); otherwise it will go as chunked encoding which is slightly larger and a little more involved.

choose [
route "/plaintext" >=> text "Hello, World!"
route "/json" >=> json { message = "Hello, World!" }
route "/jsonutf8" >=> jsonUtf8 { message = "Hello, World!" }

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Will need to add a second dockerfile for this second json route (that's identical to first)
giraffe-utf8json.dockerfile

Then add a new entry in benchmark_config.json for the json

"utf8json": {
    "json_url": "/jsonutf8",
   ...
"json_url": "/json",
"port": 8080,
"approach": "Realistic",
"classification": "Full",

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Needs to be Fullstack

<PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="2.1.0" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.Cookies" Version="2.1.0" />

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Some of these PackageReferences can be trimmed

@@ -0,0 +1,25 @@


This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Don't need the sln file; up to you though

open System
open Microsoft.AspNetCore.Builder
open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Logging

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Some of these can be trimmed

@dv00d00

This comment has been minimized.

Contributor

dv00d00 commented Jun 12, 2018

I will need to check if docker actually able to run the app. It died on my work machine after latest windows update

}
},
{
"jsonutf8": {

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

nit: Call it utf8json to match the library name?

Then it will appear in the listings as giraffe-utf8json as per https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/CSharp/aspnetcore/benchmark_config.json#L82

Url can remain the same

},
{
"jsonutf8": {
"plaintext_url": "/plaintext",

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

nit: Don't need plaintext entry for this one as nothing is different for plaintext, so no reason to test it twice

[<CLIMutable>]
type JsonMessage = { message : string }
let jsonutf8 (data:obj) : HttpHandler =

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Should this be data: JsonMessage?

Also is type JsonMessage the equivalent of a C# struct type? (rather than a class type)

The generic Utf8Json Serialize overload for will work better for the struct type (don't change it for the regular one as Json.Net would box it to obj anyway...)

This comment has been minimized.

@dv00d00

dv00d00 Jun 12, 2018

Contributor

It should be a class
Will add struct for utf8 case + make it generic

@benaadams

This comment has been minimized.

Contributor

benaadams commented Jun 12, 2018

Some nits, but beautiful!

@benaadams

This comment has been minimized.

Contributor

benaadams commented Jun 12, 2018

I will need to check if docker actually able to run the app. It died on my work machine after latest windows update

I run it in a linux VM on Windows, seems easiest as the tests set up a bunch of docker containers.

You can verify it locally with

sudo ./tfb --test=giraffe --mode=verify

And locally benchmark (e.g. plaintext) with

sudo ./tfb --test=giraffe --type=plaintext

Are also docs on the benchmarks https://frameworkbenchmarks.readthedocs.io/

@@ -0,0 +1,12 @@
FROM microsoft/dotnet:2.1-sdk-stretch AS build

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

need to rename this docker file to match name change
giraffe-jsonutf8.dockerfile -> giraffe-utf8json.dockerfile

This comment has been minimized.

@dv00d00

dv00d00 Jun 12, 2018

Contributor

oops

@benaadams

This comment has been minimized.

Contributor

benaadams commented Jun 12, 2018

Don't forget to remove WIP from the title :)

@dv00d00 dv00d00 changed the title from WIP FSharp giraffe to FSharp giraffe Jun 12, 2018

@dv00d00

This comment has been minimized.

Contributor

dv00d00 commented Jun 12, 2018

Out of scope of this PR, but if, let's say, I'll port asp.net core mvc app to F#, will it make any sense?

@benaadams

This comment has been minimized.

Contributor

benaadams commented Jun 12, 2018

I'll port asp.net core mvc app to F#, will it make any sense?

Maybe? Can add it as a mvc branch in benchmark_config.json with it own dockerfile as per utf8json; may want to have it as a separate fsprog.

The aspnetcore version is a little complicated as it has every permutation in it; then uses command line params to determine what to set up so you may want to go simplier, unless you want to switch for all the databases, different orms and middleware vs mvc.

@benaadams

This comment has been minimized.

Contributor

benaadams commented Jun 12, 2018

@dv00d00 might want to signal when you are complete with the PR, so the techempower peeps know when to merge

@dv00d00 dv00d00 changed the title from FSharp giraffe to [WIP] FSharp giraffe Jun 12, 2018

@dv00d00

This comment has been minimized.

Contributor

dv00d00 commented Jun 12, 2018

will update it shortly, going to split stock and custom implementations

Dmitry Kushnir
removed boxing from utf8json handler
separated custom implementation from stock

@dv00d00 dv00d00 changed the title from [WIP] FSharp giraffe to FSharp giraffe Jun 12, 2018

@dv00d00

This comment has been minimized.

Contributor

dv00d00 commented Jun 12, 2018

I think it is ok now

@benaadams

This comment has been minimized.

Contributor

benaadams commented Jun 12, 2018

F#, welcome .NET Player 2 😃

image

/cc @nbrady-techempower

BDN.Generated/
binaries/
global.json
*.sln

This comment has been minimized.

@msmith-techempower

msmith-techempower Jun 12, 2018

Member

With the Docker implementation, .gitignore should not be needed.

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Makes it hard to edit the project in Visual Studio and use git without it (as VS creates lots of support files)

This comment has been minimized.

@msmith-techempower

@msmith-techempower msmith-techempower merged commit 2669ae4 into TechEmpower:master Jun 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isaacabraham

This comment has been minimized.

isaacabraham commented Jun 12, 2018

What's the CLIMutable needed for?

<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.Hosting" Version="2.1.0" />
<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.1.0" />

This comment has been minimized.

@forki

forki Jun 12, 2018

Can we get rid of DI?

| [| "stock" |] -> Stock
| _ -> Custom
printfn "Running with %A implementation" implementation

This comment has been minimized.

@forki

forki Jun 12, 2018

Printf is usually very slow. I'm not sure if it's part of the benchmark, but it should not be used in hot paths.

This comment has been minimized.

@forki

forki Jun 12, 2018

Console.WriteLine would do

let custom : HttpHandler list =
let inline contentLength (x:int32) = new System.Nullable<int64>( int64 x )
let json data : HttpHandler =

This comment has been minimized.

@forki

forki Jun 12, 2018

I think we should use the giraffe helpers here.

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Helpers are already used in stock? (idiomatic)

Custom uses a static conversion for plaintext, and utf8json for json lib (benchmark optimized)

Both varieties will show up in the benchmarks

This comment has been minimized.

@forki

forki Jun 12, 2018

Ah didn't see that. Thanks

ctx.WriteBytesAsync bytes
let bytes = System.Text.Encoding.UTF8.GetBytes "Hello, World!"
let text : HttpHandler =

This comment has been minimized.

@forki

forki Jun 12, 2018

I think we should use giraffe helpers here.

This comment has been minimized.

@benaadams

benaadams Jun 12, 2018

Contributor

Helpers are used for this type in stock (idiomatic)

open Microsoft.AspNetCore.Hosting
open Giraffe
[<CLIMutable>]

This comment has been minimized.

@forki

forki Jun 12, 2018

Mhm. Not 100 percent sure, but usually we don't need cli mutable. I don't have it anywhere in any of my giraffe apps

@forki

This comment has been minimized.

forki commented Jun 12, 2018

Very cool stuff. I added some smaller comments. Maybe you want to move it a bit more in direction of idiomatic giraffe.

@forki

This comment has been minimized.

forki commented Jun 12, 2018

I also recommend that you you try to use giraffe's tokenrouter if you didn't try it yet. It should be much faster than the "old" suave style routing

@dv00d00

This comment has been minimized.

Contributor

dv00d00 commented Jun 13, 2018

thanks for the review @forki

pritnfn is called once per app startup, so should not cause any issues.
As for TokenRouter, it does look good, but I think we can't use prerelease packages

@forki

This comment has been minimized.

forki commented Jun 13, 2018

@gerardtoconnor

This comment has been minimized.

Contributor

gerardtoconnor commented Jun 13, 2018

On NuGet it's saying it's a beta pre-release, unfortunately :( . I don't release many NuGet packages, do we remove pre-release state, by removing "beta" from project version?

Given this is a performance test we should probably avoid CE, any other fancy syntax and any unneeded runtime ops like filtering GET, sticking to explicit perf code as no-one references these benchmarks for building systems, just getting perf tips?

@forki

This comment has been minimized.

forki commented Jun 13, 2018

@gerardtoconnor

This comment has been minimized.

Contributor

gerardtoconnor commented Jun 16, 2018

Did review in RFC Fsharp giraffe fortunes, looks good for the most part, not much more to do without changing router or implementing more explicit json writer. 👍

roberthusak added a commit to roberthusak/FrameworkBenchmarks that referenced this pull request Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment