Coroutinize http client code #2429

xemul · 2024-09-12T12:00:35Z

The code looks nicer and with much less indentation depth that way

And brush up the if-else nesting a little bit to avoid co_return-s Signed-off-by: Pavel Emelyanov <[email protected]>

For simplicity, wrap the entrance `if` around. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

And leave indentation broken until next patch Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

The former helper was introduced to reduce the netsing of .then-chains. Now when it's a coroutinie, it's better be squashed with its only caller. Signed-off-by: Pavel Emelyanov <[email protected]>

Same as previous patch -- the former method was a way to reduce indentation level for the latter. Now they look good enough when squashed. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

There's a trickery there. After _closed future is resolved "this" may become freed already. So to make the last log message printed, the this->local_address was saved into .then capture. When coroutinized, that field is kept on stack. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

There's a trickery here. After con->close() is resolved, the con may be the last pointer holding the connection alive. Before this patch, it was saved on .finally capture, after it -- it's preserved as on-stack variable. Signed-off-by: Pavel Emelyanov <[email protected]>

In its .then-chain form the method calls itself to emulate "goto again" without goto. To do the same with coroutines, wrap it into a loop. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

This method closes all its connection by calling itself until the pool is empty. With coroutines it is done with the plain loop. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

tchaikov · 2024-09-12T15:59:01Z

include/seastar/http/client.hh

@@ -191,7 +191,7 @@ private:
    future<> shrink_connections();

    template <std::invocable<connection&> Fn>
-    auto with_connection(Fn&& fn, abort_source*);
+    futurize_t<std::invoke_result_t<Fn, connection&>> with_connection(Fn fn, abort_source*);


heh, i believe Avi would like this change.

It has less than 4 closing corner braces in a row, so not that bad

Meanwhile in scylla (scroll right):

test/boost/user_function_test.cc:769: e.execute_cql("CREATE FUNCTION my_func(val int) CALLED ON NULL INPUT RETURNS map<int, frozen<set<frozen<list<frozen<tuple<text, bigint>>>>>>> LANGUAGE Lua AS \

😱

ah, i meant, he always prefers an explicit return type. this one make it more explicit. so..

src/http/client.cc

tchaikov · 2024-09-12T16:09:06Z

src/http/client.cc

@@ -341,13 +341,11 @@ future<> client::do_make_request(connection& con, request& req, reply_handler& h
        connection::reply_ptr reply = co_await con.do_make_request(req);
        auto& rep = *reply;
        if (expected.has_value() && rep._status != expected.value()) {
-            if (!http_log.is_enabled(log_level::debug)) {
-                throw httpd::unexpected_status_error(reply->_status);
+            if (http_log.is_enabled(log_level::debug)) {


i'd prefer early return for less indent. but don't feel strong either way.

This is to de-duplicate two throw-s of the same exception

tchaikov

left some comments.

src/http/client.cc

Signed-off-by: Pavel Emelyanov <[email protected]>

When reply contains unexpected status, client may print its body in logs. Before being coroutinized it was two disctinct chains, with coroutines it makes sense to rewrap them a little bit. Signed-off-by: Pavel Emelyanov <[email protected]>

It used to call itself to "goto again". With coroutines it won't work as nice, neither it looks good enough with the outer loop :( So use goto explicitly. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

It makes some non-trivial decisions on whether or to retry the request in the catch block, so the conversion is a bit hairy. Signed-off-by: Pavel Emelyanov <[email protected]>

Signed-off-by: Pavel Emelyanov <[email protected]>

xemul · 2024-09-12T17:05:10Z

upd:

removed unused ex variable
simplified retry coroutinization in client::do_make_request (con-less overload) patch

tchaikov

lgtm

gleb-cloudius · 2024-09-15T07:54:38Z

As a general note when I tried to co-routinize cql server in Scylla I did not find a way to do so without introducing more allocations when requests are pipelined. With continuations if we read more than one request from a socket all of them can be processed without allocating continuations since futures will be ready. With co-routins we always allocate.

avikivity · 2024-09-16T17:00:51Z

With [[clang::coro_elide_safe]] nested coroutines can share the coroutine frame. However it only arrives in clang 20 and isn't standardized.

avikivity · 2024-09-16T17:01:46Z

src/http/client.cc

-        return _write_buf.write(req.content);
-    } else {
-        return make_ready_future<>();
+        co_await _write_buf.write(req.content);


Doesn't this add an allocation in the common case that the buffer isn't full?

Yes, but all users we have use lambda body writer and don't step on this branch. But lambda body writer also adds allocation here

avikivity · 2024-09-16T17:03:21Z

src/http/client.cc

-    if (req.get_header("Expect") == "") {
-        return make_ready_future<reply_ptr>(nullptr);
+    if (req.get_header("Expect") != "") {
+        co_await _write_buf.flush();


The common case is, that Expect will be empty, no? so we add an allocation here.

Please measure the performance of the series.

The common case is, that Expect will be empty, no? so we add an allocation here.

The allocation is unavoidable here anyway, at the end this method does co_await recv_reply() and it will wait for it

Please measure the performance of the series.

ok

Please measure the performance of the series.

With #2442 , the test makes 1M requests in

master: 3.40 +/- 0,02 seconds (min 3,37 / max 3,50)

this PR: 3,48 +/- 0,02 seconds (min 3,45 / max 3,62)

Please report allocs/op

master: 45 allocs/req

this PR: 46 allocs/req

nyh · 2024-09-17T16:04:13Z

Hi @xemul,

When you created a 28-patch series which must have taken you many many hours to create, please spend another 10 minutes trying to explain your motivation for doing this. Was it just the generic "coroutines are nicer" sentiment that we all shared, or something specific which bothered you?

Also, I see a lot of discussion regarding the lower performance of the coroutine code, so I wonder if you made an attempt to benchmark the http server somehow to see if there's anything we need to worry about.

xemul · 2024-09-18T16:04:38Z

Hi @xemul,

When you created a 28-patch series which must have taken you many many hours to create, please spend another 10 minutes trying to explain your motivation for doing this. Was it just the generic "coroutines are nicer" sentiment that we all shared, or something specific which bothered you?

No, nothing specific. It just looks much nicer and simpler this way.

Also, I see a lot of discussion regarding the lower performance of the coroutine code, so I wonder if you made an attempt to benchmark the http server somehow to see if there's anything we need to worry about.

I made a silly test with existing http client demo, but it's not very representative. I'll update the discussion thread with better measurement

xemul requested review from tchaikov and nyh September 12, 2024 12:00

xemul added 22 commits September 12, 2024 15:25

http/client: Coroutinize connection::write_body

797dc34

And brush up the if-else nesting a little bit to avoid co_return-s Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize connection::maybe_wait_for_continue

32365d4

For simplicity, wrap the entrance `if` around. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize connection::send_request_head

e8df44b

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize connection::recv_reply

dd51c2c

And leave indentation broken until next patch Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of connection::recv_reply

ad9833d

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize connection::do_make_request

9b46ea5

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of connection::do_make_request

86d6a8b

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Squash maybe_wait_for_continue into do_make_request

14e3efc

The former helper was introduced to reduce the netsing of .then-chains. Now when it's a coroutinie, it's better be squashed with its only caller. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client Squash send_request_head into do_make_request

c5a63be

Same as previous patch -- the former method was a way to reduce indentation level for the latter. Now they look good enough when squashed. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize connection::make_request

9b473bf

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of connection::close

cb1b4b6

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::make_connection

bd36cf3

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of client::make_connection

bdfdf16

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::shrink_connections

215e024

In its .then-chain form the method calls itself to emulate "goto again" without goto. To do the same with coroutines, wrap it into a loop. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of client::shrink_connections

88de135

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::set_maximum_connections

b10c8b6

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::with_connection

685f6f2

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::with_new_connection

200ea10

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::close

4250bca

This method closes all its connection by calling itself until the pool is empty. With coroutines it is done with the plain loop. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of client::close

688ac04

Signed-off-by: Pavel Emelyanov <[email protected]>

xemul force-pushed the br-http-client-coroutinize branch from 1618829 to 21a5b8c Compare September 12, 2024 12:25

tchaikov reviewed Sep 12, 2024

View reviewed changes

src/http/client.cc Show resolved Hide resolved

xemul added 2 commits September 12, 2024 20:00

http/client: Coroutinize client::do_make_request

3e22a25

Signed-off-by: Pavel Emelyanov <[email protected]>

xemul added 4 commits September 12, 2024 20:00

http/client: Coroutinize client::get_connection

4dfb090

It used to call itself to "goto again". With coroutines it won't work as nice, neither it looks good enough with the outer loop :( So use goto explicitly. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of client::get_connection

539fa2a

Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Coroutinize client::do_make_request (con-less overload)

3231966

It makes some non-trivial decisions on whether or to retry the request in the catch block, so the conversion is a bit hairy. Signed-off-by: Pavel Emelyanov <[email protected]>

http/client: Restore indentation of client::do_make_request overload

67134fd

Signed-off-by: Pavel Emelyanov <[email protected]>

xemul force-pushed the br-http-client-coroutinize branch from 21a5b8c to 67134fd Compare September 12, 2024 17:03

tchaikov approved these changes Sep 13, 2024

View reviewed changes

avikivity reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coroutinize http client code #2429

Coroutinize http client code #2429

xemul commented Sep 12, 2024 •

edited

Loading

tchaikov Sep 12, 2024

xemul Sep 12, 2024

tchaikov Sep 13, 2024

tchaikov Sep 12, 2024

xemul Sep 12, 2024

tchaikov left a comment

xemul commented Sep 12, 2024

tchaikov left a comment

gleb-cloudius commented Sep 15, 2024

avikivity commented Sep 16, 2024

avikivity Sep 16, 2024

xemul Sep 17, 2024

avikivity Sep 16, 2024

xemul Sep 17, 2024

xemul Sep 18, 2024

avikivity Sep 18, 2024

xemul Sep 19, 2024

nyh commented Sep 17, 2024

xemul commented Sep 18, 2024

Coroutinize http client code #2429

Are you sure you want to change the base?

Coroutinize http client code #2429

Conversation

xemul commented Sep 12, 2024 • 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

tchaikov left a comment

Choose a reason for hiding this comment

xemul commented Sep 12, 2024

tchaikov left a comment

Choose a reason for hiding this comment

gleb-cloudius commented Sep 15, 2024

avikivity commented Sep 16, 2024

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

nyh commented Sep 17, 2024

xemul commented Sep 18, 2024

xemul commented Sep 12, 2024 •

edited

Loading