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

Migrate to async futures with reqwest #46

Merged
merged 5 commits into from May 14, 2020
Merged

Migrate to async futures with reqwest #46

merged 5 commits into from May 14, 2020

Conversation

messense
Copy link
Contributor

No description provided.

@messense messense mentioned this pull request May 10, 2020
@fcsonline
Copy link
Owner

Wow! Awesome job! 👏

Let me test it with some common use cases to give you come feedback.

@fcsonline
Copy link
Owner

In the examples/server you can start a small nodejs server to test some of the example we have. I have played with the throughput.yml one, tweaking the number of threads and iterations to (10 and 1000 respectively) and I have seen those numbers.

Master

Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   3ms
Fetch users               Average time per request  3ms
Fetch users               Sample standard deviation 1ms

Concurrency Level         10
Time taken for tests      3.0 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       3321.76 [#/sec]
Median time per request   3ms
Average time per request  3ms
Sample standard deviation 1ms

Async:

Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   17ms
Fetch users               Average time per request  19ms
Fetch users               Sample standard deviation 6ms

Concurrency Level         10
Time taken for tests      21.3 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       469.37 [#/sec]
Median time per request   17ms
Average time per request  19ms
Sample standard deviation 6ms

Do you know the reason why the number of requests per second is much lower?

Cargo.toml Show resolved Hide resolved
@messense
Copy link
Contributor Author

Do you know the reason why the number of requests per second is much lower?

Yes, it is expected to be slower in its current form, it still runs requests sequentially in each thread which isn't ideal. This patch is the minimal changes to make drill work with async-await, refactoring needed to actually utilize the async runtime.

@messense
Copy link
Contributor Author

@fcsonline Can you try again with latest commit? On my local machine (MacBook Pro (15-inch, 2018) 2.2 GHz 6-Core Intel Core i7 16 GB 2400 MHz DDR4) the result is:

drill sync

Threads 10
Iterations 1000
Rampup 0
Base URL http://localhost:9000


Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   23ms
Fetch users               Average time per request  32ms
Fetch users               Sample standard deviation 104ms

Concurrency Level         10
Time taken for tests      32.0 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       312.92 [#/sec]
Median time per request   23ms
Average time per request  32ms
Sample standard deviation 104ms

drill async

Threads 10
Iterations 1000
Rampup 0
Base URL http://localhost:9000


Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   17ms
Fetch users               Average time per request  21ms
Fetch users               Sample standard deviation 16ms

Concurrency Level         10
Time taken for tests      21.6 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       463.52 [#/sec]
Median time per request   17ms
Average time per request  21ms
Sample standard deviation 16ms

@messense
Copy link
Contributor Author

I've tested on a Linux server, async version is still slower.

Sync

Threads 10
Iterations 1000
Rampup 0
Base URL http://localhost:9000


Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   2ms
Fetch users               Average time per request  2ms
Fetch users               Sample standard deviation 1ms

Concurrency Level         10
Time taken for tests      2.2 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       4543.37 [#/sec]
Median time per request   2ms
Average time per request  2ms
Sample standard deviation 1ms

Async

Threads 10
Iterations 1000
Rampup 0
Base URL http://localhost:9000


Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   2ms
Fetch users               Average time per request  2ms
Fetch users               Sample standard deviation 0ms

Concurrency Level         10
Time taken for tests      9.8 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       1018.73 [#/sec]
Median time per request   2ms
Average time per request  2ms
Sample standard deviation 0ms

@messense
Copy link
Contributor Author

By keep reqwest::Client around instead of recreating it on every iteration the async version's performance is now on par with sync

Sync:

Threads 10
Iterations 1000
Rampup 0
Base URL http://localhost:9000


Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   2ms
Fetch users               Average time per request  2ms
Fetch users               Sample standard deviation 1ms

Concurrency Level         10
Time taken for tests      2.2 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       4538.68 [#/sec]
Median time per request   2ms
Average time per request  2ms
Sample standard deviation 1ms

Async

Threads 10
Iterations 1000
Rampup 0
Base URL http://localhost:9000


Fetch users               Total requests            10000
Fetch users               Successful requests       10000
Fetch users               Failed requests           0
Fetch users               Median time per request   2ms
Fetch users               Average time per request  2ms
Fetch users               Sample standard deviation 1ms

Concurrency Level         10
Time taken for tests      2.0 seconds
Total requests            10000
Successful requests       10000
Failed requests           0
Requests per second       5046.84 [#/sec]
Median time per request   2ms
Average time per request  2ms
Sample standard deviation 1ms

Client::new()
};

// let client = ClientBuilder::default().danger_accept_invalid_certs(config.no_check_certificate).build().unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs a little refactor to pass no_check_certificate config to reqwest::Client in Request::new

Copy link
Owner

Choose a reason for hiding this comment

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

Don't worry about it for now. After merging this async branch I want to start dealing with the keep-alive feature. It is a requirement for a long time that will increase the throughput drastically.

The idea is to have a pool of clients, (domain -> Client), to be used here. The domain will be determined from the interpolated_url.

Copy link
Owner

@fcsonline fcsonline May 12, 2020

Choose a reason for hiding this comment

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

I created this branch forked form this one to play a bit. e6b51ff

Keep Alive is awesome! 💪

@fcsonline
Copy link
Owner

fcsonline commented May 13, 2020

Unlocking async will be an awesome milestone for drill. I have been testing and I'm really happy with the current performance. Right after this, I can open the keep-alive pull request(e6b51ff) that will improve even more throughput. Also, it enables two more branches I'm working on.

What do you think about merging this?

@messense
Copy link
Contributor Author

@fcsonline Let's merge it.

@fcsonline fcsonline merged commit cab8ead into fcsonline:master May 14, 2020
@messense messense deleted the async branch May 15, 2020 03:02
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.

None yet

2 participants