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

Fix fsharp giraffe benchmarks #5929

Conversation

SIRHAMY
Copy link
Contributor

@SIRHAMY SIRHAMY commented Dec 6, 2022

This commit aims to rectify oddities in the fsharp giraffe benchmarks (giraffe, giraffe-endpoints) that likely led to its unusually poor results.

The fix to each is to clear the default logging providers - presumably this removes a good amount of processing / IO which leads to better request / response performance.

Why is this okay? Is this cheating?

I believe this is okay because this seems to be the standard for dotnet benchmarks (fsharp and csharp) - both in this repo and in others (like tech empower).

It is a little sus (idk if other languages are doing this) but it will at least get these benchmarks in parity with others on the same tech stack.

fsharp:

csharp:

aspnet-minimal-api - clears - https://github.com/the-benchmarker/web-frameworks/blob/master/fsharp/websharper/Main.fs#L44

tech empower

References

@waghanza
Copy link
Collaborator

waghanza commented Dec 6, 2022

Thanks @SIRHAMY. Seems ok for me. Indeed, for benchmarking purposes, all IO (mostly logging) should be disabled 👍🏻

@SIRHAMY
Copy link
Contributor Author

SIRHAMY commented Dec 6, 2022

Ah good to know!

Is there anything else we need to do to get this code accepted and merged?

@waghanza
Copy link
Collaborator

waghanza commented Dec 6, 2022

will wait a bit for @panesofglass review, and merge (maybe in few days)

results are updated weekly (at least I try to do this)

@waghanza waghanza enabled auto-merge (squash) December 7, 2022 12:46
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.

2 participants