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.

Sign up for GitHub

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

Jump to bottom

Add Rust hyper http benchmark #1043

Merged
merged 4 commits into from
Oct 21, 2018
Merged

Add Rust hyper http benchmark #1043

merged 4 commits into from
Oct 21, 2018

Conversation

kevinkassimo
Copy link
Contributor

No description provided.

ry
ry reviewed Oct 20, 2018
View reviewed changes
tools/http_benchmark.py Outdated
go_build_cmd = ["go", "build", "-o", tmp_prog, "tools/go_net_http.go"]
subprocess.call(go_build_cmd)
print "http_benchmark testing GO net/http."
return run([tmp_prog, ADDR.split(":")[1]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding Go as a dep is pretty heavy - it's a whole new runtime that we don't use currently. Could you instead use a hyper http server - EG https://github.com/hyperium/hyper/blob/master/examples/hello.rs
You can add a new rust_binary("hyper_hello") to //BUILD.gn
In both rust and go, they are going to be an order of magnitude or more faster than we can ever achieve with a single threaded dynamic runtime. However it will provide something to strive for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

@kevinkassimo kevinkassimo changed the title [WIP] Add go net/http benchmark [WIP] Add Rust hyper http benchmark Oct 20, 2018
@kevinkassimo kevinkassimo changed the title [WIP] Add Rust hyper http benchmark Add Rust hyper http benchmark Oct 20, 2018
ry
ry reviewed Oct 20, 2018
View reviewed changes
BUILD.gn Outdated
@@ -137,6 +138,11 @@ rust_executable("deno_ns") {
]
}

rust_executable("hyper_hello") {
source_root = "tools/hyper_hello.rs"
extern = main_extern
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

extern = [ "$rust_build:hyper" ]

tools/http_benchmark.py Outdated
return {
"deno": deno_rps,
"node": node_rps,
"rust hyper": rust_hyper_http_rps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This is going to be humbling.

ry
ry reviewed Oct 20, 2018
View reviewed changes
tools/http_benchmark.py Outdated
return {
"deno": deno_rps,
"node": node_rps,
"rust hyper": rust_hyper_http_rps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/rust hyper/hyper/
I think we should keep these label names short and without spaces.

ry
ry reviewed Oct 20, 2018
View reviewed changes
tools/http_benchmark.py Outdated
def http_benchmark(deno_exe):
deno_rps = deno_http_benchmark(deno_exe)
node_rps = node_http_benchmark(deno_exe)
def rust_hyper_http_benchmark(hyper_hello_exe):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/rust_hyper_http_benchmark/hyper_http_benchmark/

@kevinkassimo
Copy link
Contributor Author

For some reason I could not pass AppVeyor build after reducing extern to only $rust_build:hyper

Also, in my benchmarks on both Fedora and Mac machines, I noticed that while Req/Sec is better than Node, Transfer/Sec is only about half. Probably need to benchmark this also

@ry
Copy link
Member

ry commented Oct 20, 2018

@kevinkassimo libc would be my guess too. Let's see if that fixes it.

Regarding the benchmarks - we might have to debug it a bit. Were you using the release build?

@kevinkassimo
Copy link
Contributor Author

@ry Yeah it’s release build.

@kevinkassimo
Copy link
Contributor Author

On my Fedora machine:

http_benchmark testing DENO.
Listening on 127.0.0.1:4544
third_party/wrk/linux/wrk -d 10s http:https://127.0.0.1:4544/
Running 10s test @ http:https://127.0.0.1:4544/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   539.66us  400.92us   8.65ms   96.40%
    Req/Sec    10.04k     1.48k   11.80k    71.00%
  199876 requests in 10.00s, 9.72MB read
Requests/sec:  19985.60
Transfer/sec:      0.97MB

http_benchmark testing NODE.
port 4544
third_party/wrk/linux/wrk -d 10s http:https://127.0.0.1:4544/
Running 10s test @ http:https://127.0.0.1:4544/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   594.70us  379.54us  16.42ms   97.71%
    Req/Sec     8.65k     1.07k    9.98k    92.08%
  173874 requests in 10.10s, 18.57MB read
Requests/sec:  17215.06
Transfer/sec:      1.84MB

http_benchmark testing RUST hyper.
Listening on http:https://127.0.0.1:4544
third_party/wrk/linux/wrk -d 10s http:https://127.0.0.1:4544/
Running 10s test @ http:https://127.0.0.1:4544/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   428.37us    1.15ms  14.46ms   93.09%
    Req/Sec    32.46k     6.60k   45.82k    63.00%
  648053 requests in 10.05s, 54.39MB read
Requests/sec:  64492.15
Transfer/sec:      5.41MB

@ry
Copy link
Member

ry commented Oct 20, 2018

Kevin, that looks about right. Probably node has more headers.

@kevinkassimo
Copy link
Contributor Author

Hmm yeah, might be the reason.
We should probably update the benchmarks once HTTP is implemented.

@kevinkassimo kevinkassimo force-pushed the benchmark/go_net_http branch 8 times, most recently from 487200b to 0f936d5 Compare October 21, 2018 02:34
@kevinkassimo
Copy link
Contributor Author

@ry it was $rust_build:ring for some reason... Build finally passes on windows...

ry
ry approved these changes Oct 21, 2018
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - cool thanks for this

@ry ry merged commit 86409eb into denoland:master Oct 21, 2018
@kitsonk
Copy link
Contributor

kitsonk commented Oct 21, 2018

This did not go through ./tools/format.py and now when run on master it modified files.

We really need lack of this to fail the build.

@kevinkassimo
Copy link
Contributor Author

@kitsonk sorry about that... I probably forget to run format again as I was busy addressing issues and extern problems...

Adding a guard against this should definitely help

@ry
Copy link
Member

ry commented Oct 21, 2018

Now that we have rustfmt in third party it should be easier to do this now.

@ghost
Copy link

ghost commented Oct 21, 2018

Awesome - now you have a proper reference point to work towards 👍

@kevinkassimo kevinkassimo deleted the benchmark/go_net_http branch December 27, 2019 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kevinkassimo @ry @kitsonk