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

feat: add turbo-http to benchmark suite #47

Closed
wants to merge 1 commit into from

Conversation

tuananh
Copy link

@tuananh tuananh commented Mar 5, 2018

@mcollina
Copy link
Member

mcollina commented Mar 5, 2018

I'm 👎 to this change right now for two reason:

  1. autocannon is too slow to benchmark turbo-http. We are integrating turbo-net there, and once that is available we can revisit. Basically autocannon would become the bottleneck to benchmark turbo-http.

  2. This repository is a comparison of web frameworks built on top of Node.js core http stack. Changing the http implementation is something an order of magnitude more disruptive than changing framework.

cc @mafintosh

@StarpTech
Copy link
Member

I'm 👍 with @mcollina but point 2 should be documented. In the current state, we benchmark any mature HTTP framework for Node.js.

@mcollina
Copy link
Member

mcollina commented Mar 5, 2018

Adding alternative http implementation for node is something that could be done with our harness. But it's a completely different category of problems. Node.js core is only adding here as a base reference.

@lukeed
Copy link
Contributor

lukeed commented Mar 5, 2018

This isn't a framework. All current benchmarks are based off native HTTP module for equal playing field. Turbo is made to replace the HTTP module itself, not add routing, middleware, etc, on top of HTTP.

@StarpTech
Copy link
Member

@lukeed this is not the point. We also bench e.g micro, trek without and with a router.

@lukeed
Copy link
Contributor

lukeed commented Mar 5, 2018

Upgrading HTTP will affect all frameworks, regardless of implementation. It's meaningless to compare 20+ frameworks running on Node HTTP with one running on a C-based HTTP. Doing that is comparing Node to C, or Node to Go, etc.

This benchmark suite is to compare frameworks (or partial frameworks), measuring their speed and their overhead.. not the underlying HTTP server.

It'd also be similar to comparing Node 0.12 vs Node 10.0 --- they're completely different. For example, I can run Fastify on 0.12 for X results. Then run Express on Node 10.0 for Y results. Or even run Express on turbo-http for Z results. Y and Z will show that Express "is faster than" Fastify, which most definitely isn't the case.

As I'm sure you well know: You have to standardize machine environments and Node environments for fair testing. The HTTP server that powers the benchmark is no different but it, too, is the environment.

@StarpTech
Copy link
Member

Wooah.. @mcollina said it well. We won't add it to the benchmarks. We just have to update the docs 😄 My statement was based on not add routing, middleware, etc, on top of HTTP.

@lukeed
Copy link
Contributor

lukeed commented Mar 5, 2018

Wooah..

If there was any hostility of any kind, it wasn't intended -- sorry! The drawbacks of text-only conversation 😆I was just trying to explain my point fully. Apologies

@StarpTech
Copy link
Member

No problem this is just a discussion. We all agree 👍

@aichholzer
Copy link
Contributor

My two cents; I believe turbo should not be part of this benchmarks. It's indeed like comparing apples to oranges.

In either case, @mcollina -I suggest we close this for now, at least until autocannon is turbo-ready and then we can re-boot the discussion as to whether it fits in here or not.

@mcollina mcollina closed this Jun 12, 2018
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.

5 participants