search

tailhook / rotor-http

The mio/rotor based http server library for rust (UNMAINTAINED, use tk-http)
MIT License
110 stars 11 forks source link

counter is now thread safe #33

Closed yanns closed 8 years ago

yanns commented 8 years ago

warning: I am a rust beginner and implement that in the rust hack & tell... ;)

tailhook commented 8 years ago

Well, from the Rust point of view your code is fine.

But we use "threaded" example for benchmarking. And it's expected for a single counter which is modified by every request to be a bottleneck. It's definitely not expected for multi-threaded application, which could benefit from the parallelism to have such single synchronization place.

Have you tried to run wrk or any other http benchmarking tool on the server before and after the change? It's interesting on how it influences, at least for microbenchmarks.

yanns commented 8 years ago

good to know that the code is fine from the Rust point of view :smiley:

Before:

wrk -t8 -c 400 http://localhost:3000/
Running 10s test @ http://localhost:3000/
  8 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     8.07ms    1.54ms  25.34ms   77.40%
    Req/Sec     5.94k   660.86     7.96k    80.25%
  472805 requests in 10.02s, 23.00MB read
  Socket errors: connect 0, read 255, write 0, timeout 0
Requests/sec:  47173.28
Transfer/sec:      2.29MB

After:

wrk -t8 -c 400 http://localhost:3000/
Running 10s test @ http://localhost:3000/
  8 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     7.92ms    1.37ms  21.04ms   66.57%
    Req/Sec     5.85k   511.69     7.16k    77.25%
  465989 requests in 10.01s, 22.66MB read
  Socket errors: connect 0, read 252, write 0, timeout 0
Requests/sec:  46532.18
Transfer/sec:      2.26MB

So there is quite a difference. If you use this as benchmark, then I'd suggest not to merge this.

tailhook commented 8 years ago

Well, it would be fine if you just copy an example and create a threaded_with_shared_counter.rs, or something like that. Benchmarking both is actually interesting, especially for machines with the larger number of cores.

yanns commented 8 years ago

OK, I copied the example and added a comment why this file exist. Thx for the feedback!

tailhook commented 8 years ago

Well, it looks like you're using old version of the example https://travis-ci.org/tailhook/rotor-http/jobs/115569453#L333 master branch is already changed a bit.

yanns commented 8 years ago

Now it's green on rust stable and rust beta, but still fails with rust nightly.

tailhook commented 8 years ago

Yes. Crash in nightly is not your fault. I've squashed commits and merged in. Thanks!