(feat) reenable benches/end_to_end.rs #2937

Robertoskr · 2022-08-09T19:25:37Z

This benches were disabled becuase of the deprecation of Client and Server,
this pr is to reenable them with the new conn aproach

Robertoskr · 2022-08-09T19:38:04Z

benches/end_to_end.rs

+            for _ in 0..self.parallel_cnt {
+                b.iter(|| {
+                    let req = make_request();
+                    rt.block_on(send_request(req));
+                });
+            }


as you can see @seanmonstar we do this in sync way... If i try to run this async way, i will have an error because the connection is not ready...
What is the new approach for handling parallel request with the new "conn" things? I tried with poll_ready, to wait until the connection is ready before making the requests...
What do you think is the best here ? Opening n connections to the server or using an approach like using the poll ready function ?

also i see on src/client/conn/mod.rs in the example this line:
request_sender.ready().await?;
wich i think is wrong because i can't find that method on the code 💢 (maybe im wrong)

Yea, we don't want to use block_on there. Especially since the parallel option is meant to send them all at the same time.

For HTTP/1, you'd need to make parallel_cnt amount of connections, and then spawn tasks looping on ready() and then send_request().

For HTTP/2, you can make just one connection, and send parallel_cnt requests at a time.

Hey, already did the changes, as you can see in the pr, but when i run the benchs, have the following error, do you know how to solve it ? How is the best way to await the request sender until is ready to send a request ?

Hey Robertoskr. When I was working on this I called the ready method from tower::ServiceExt on the request sender: request_sender.ready().await. From the comments above it seems like you tried this, but maybe you forgot to import the trait?

Thanks! Just tried, that and works, but also tried with a tokio::sync::Lock and performs better, don't know if its safe thought, but the code works well.

and for the trait: Yes, did't know that trait existed 😹

Robertoskr · 2022-08-14T16:08:23Z

@seanmonstar @oddgrd Ready for review !

oddgrd · 2022-08-14T19:00:51Z

Hey, that's cool that you figured it out! I can have a look at this in the coming week, but I'm a new contributor myself so you'll have to wait for Sean to sign off on this whenever he has time. 🙂

oddgrd

Hey Robertoskr. I had a look at this, and when I tried to run the benchmarks on your branch it seems some of the HTTP2 ones take quite long to finish. On my computer your branch took ~290 seconds to complete, but on the commit before these benchmarks were disabled they completed in ~80s. I left a couple of comments, but unfortunately I wasn't able to solve this myself so I can't be of much help here.

If you'd like you can have a look at my attempt, maybe it will help you. I was able to get it down to ~100s, but some of the HTTP2 benches are unstable. I might keep looking into this, if I figure it out I'll let you know. Good luck!

oddgrd · 2022-08-19T12:25:21Z

benches/end_to_end.rs

+    local_addr
+}
+
+async fn handle_request(_req: Request<Body>) -> Result<Response<Body>, Infallible> {


In this function you return the request body, but you should be returning the response_body from the Opts structs.

oddgrd · 2022-08-19T12:30:40Z

benches/end_to_end.rs

+    //
+    // Benches http/1 requests
+    //


In Rust we can write doc comments with /// 🙂

oddgrd · 2022-08-19T12:41:07Z

benches/end_to_end.rs

+    let mut http = Http::new();
+    //configure http based on opts
+    http.http2_only(opts.http2)
+        .http2_initial_stream_window_size(opts.http2_stream_window)
+        .http2_initial_connection_window_size(opts.http2_conn_window)
+        .http2_adaptive_window(opts.http2_adaptive_window);


You may have forgotten to remove this code, as I don't see this variable being used anywhere.

Yep, thanks!

oddgrd · 2022-08-19T12:44:43Z

benches/end_to_end.rs

+    let (http2, http2_stream_window, http2_conn_window, http2_adaptive_window) = (
+        opts.http2,
+        opts.http2_stream_window,
+        opts.http2_conn_window,
+        opts.http2_adaptive_window,
+    );


If you #[derive(Clone)] for the Opts struct you can clone opts here and then use it directly without destructuring. (Except for the opts.response_body, which has to be destructured before being passed to the service_fn closure.)

let opts = opts.clone(); let body = opts.response_body;

Robertoskr · 2022-08-19T12:51:05Z

Hey Robertoskr. I had a look at this, and when I tried to run the benchmarks on your branch it seems some of the HTTP2 ones take quite long to finish. On my computer your branch took ~290 seconds to complete, but on the commit before these benchmarks were disabled they completed in ~80s. I left a couple of comments, but unfortunately I wasn't able to solve this myself so I can't be of much help here.

If you'd like you can have a look at my attempt, maybe it will help you. I was able to get it down to ~100s, but some of the HTTP2 benches are unstable. I might keep looking into this, if I figure it out I'll let you know. Good luck!

In my computer it runs the same as the previous version of them in 65 seconds

oddgrd · 2022-08-19T13:02:31Z

That's odd, because on my computer the http2 ones take a long time (the http1 ones run great):

seanmonstar · 2023-01-05T14:48:23Z

Thank you for tackling this issue! #3109 was just merged, which re-enables most of these benchmarks, so I'll close this one.

reenable benches/end_to_end.rs

9db5b12

Robertoskr changed the title ~~reenable benches/end_to_end.rs~~ (feat) reenable benches/end_to_end.rs Aug 9, 2022

Robertoskr commented Aug 9, 2022

View reviewed changes

Robertoskr mentioned this pull request Aug 9, 2022

Re-enable the end-to-end, pipeline, and server benchmarks #2930

Closed

3 tasks

Robertoskr added 5 commits August 9, 2022 21:58

wip

8c38687

wip

edfb77a

remove unused parameter

3c8a379

deny warnings

0c2f320

use lock in http2 requests

cbd4be3

Merge branch 'master' into feat_reenable_end_to_end_benches

3514da3

Robertoskr requested review from seanmonstar and oddgrd August 14, 2022 18:38

oddgrd reviewed Aug 19, 2022

View reviewed changes

seanmonstar linked an issue Aug 25, 2022 that may be closed by this pull request

Re-enable the end-to-end, pipeline, and server benchmarks #2930

Closed

3 tasks

seanmonstar closed this Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) reenable benches/end_to_end.rs #2937

(feat) reenable benches/end_to_end.rs #2937

Robertoskr commented Aug 9, 2022 •

edited

Loading

Robertoskr Aug 9, 2022

Robertoskr Aug 9, 2022

seanmonstar Aug 10, 2022

Robertoskr Aug 13, 2022

oddgrd Aug 14, 2022

Robertoskr Aug 14, 2022

Robertoskr Aug 14, 2022

Robertoskr commented Aug 14, 2022

oddgrd commented Aug 14, 2022 •

edited

Loading

oddgrd left a comment

oddgrd Aug 19, 2022

oddgrd Aug 19, 2022

Robertoskr Aug 19, 2022

oddgrd Aug 19, 2022 •

edited

Loading

Robertoskr Aug 19, 2022

oddgrd Aug 19, 2022

Robertoskr commented Aug 19, 2022

oddgrd commented Aug 19, 2022

seanmonstar commented Jan 5, 2023

(feat) reenable benches/end_to_end.rs #2937

(feat) reenable benches/end_to_end.rs #2937

Conversation

Robertoskr commented Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robertoskr commented Aug 14, 2022

oddgrd commented Aug 14, 2022 • edited Loading

oddgrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oddgrd Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robertoskr commented Aug 19, 2022

oddgrd commented Aug 19, 2022

seanmonstar commented Jan 5, 2023

Robertoskr commented Aug 9, 2022 •

edited

Loading

oddgrd commented Aug 14, 2022 •

edited

Loading

oddgrd Aug 19, 2022 •

edited

Loading