Optimize send path error handling #12472
Conversation
Create less of a continuation chain by handling all the exceptions in a single coroutine. Signed-off-by: Tyler Rockwood <[email protected]>
rockwotj
force-pushed
the
rockwood/optimize-exception-handling
branch
from
56c9add
to
8b5e350
Compare
|
CI Failure was #12291 which is now fixed. |
|
/ci-repeat 1 |
| .handle_exception_type([](const seastar::broken_promise&) {}) | ||
| .handle_exception_type([](const seastar::broken_condition_variable&) {}); | ||
| try { | ||
| co_await std::move(fut); |
There was a problem hiding this comment.
This seems good since handle_exception_type is relatively expensive whether an exception occurs or not, but we this transformation means we only take the exception matching cost when an exception is actually thrown.
There was a problem hiding this comment.
I guess that was the intent of this transformation?
There was a problem hiding this comment.
Based on my reading of https://github.com/redpanda-data/seastar/blob/245e0ccfa6d58d7e0dca2b4034ce1bc43e39bdc5/include/seastar/core/future.hh#L1858-L1862
It seems like before we had 5 continuations and 5 try catch blocks, and now we only have 1 of each (I think, I haven't fully wrapped my brain around how a compiler transforms coroutines). That was the intent here to cut down on both of those in a hope to lessen the cost of this function you noticed in the profiles.
src/v/rpc/transport.cc
Outdated
| if (!_dispatch_gate.try_enter()) { | ||
| return; | ||
| } | ||
| ssx::background = ssx::ignore_shutdown_exceptions(do_dispatch_send()) | ||
| .then_wrapped([this](ss::future<> fut) { | ||
| if (fut.failed()) { | ||
| vlog( | ||
| rpclog.info, | ||
| "Error dispatching socket write:{}", | ||
| fut.get_exception()); | ||
| _probe->request_error(); | ||
| fail_outstanding_futures(); | ||
| } | ||
| // Now that we've handled errors it's safe to | ||
| // release the gate. | ||
| _dispatch_gate.leave(); | ||
| }); |
There was a problem hiding this comment.
there is a slight preference to use raii gate holder rather than manual enter/leave. you can move that into the then_wrapped continuation as as capture, or dangle it off the end in a finally block capture.
There was a problem hiding this comment.
Good suggestion. dispatch_send shouldn't throw, which is why I manually used try_enter and leave, but it seems we still can check if the gate is closed before holding the gate open.
Remove the double dose of ssx::handle_shutdown_exceptions by inlining the gate handling into a single place. Remove the extra overhead of some lambdas and a couple of extra continuations by handling unexpected errors and closing the gate in a single continuation. Signed-off-by: Tyler Rockwood <[email protected]>
rockwotj
force-pushed
the
rockwood/optimize-exception-handling
branch
from
8b5e350
to
0b3fa8c
Compare
|
/backport v23.2.x |
|
/backport v23.1.x |
|
/backport v22.3.x |
|
Failed to run cherry-pick command. I executed the commands below: |
|
Failed to run cherry-pick command. I executed the commands below: |
Travis mentioned in slack that
ssx::handle_shutdown_errorswas showing up in profiles.This attempts to address that problem by creating less continuations when dispatching sends in the rpc transport layer. Additionally, switch
ssx::handle_shutdown_errorsto be a corountine.Backports Required
Release Notes