Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24176 )
Change subject: WIP [rpc] in-bulk memory recycling for Connection::ProcessOutboundTransfers() ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/24176/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24176/2//COMMIT_MSG@45 PS2, Line 45: #18 0x0000000002077de9 in kudu::rpc::Connectio > This patch addresses the issue. This call stack is captured before this ch The issue was that the InboundCall destructors were invoked upon completing a transfer as a part responding to InboundCall, interspersing I/O calls sending out the data of all the pending transfers on RPC connection. http://gerrit.cloudera.org:8080/#/c/24176/1/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: http://gerrit.cloudera.org:8080/#/c/24176/1/src/kudu/rpc/connection.cc@455 PS1, Line 455: auto* call_from_map = EraseKeyReturnValuePtr( > This might get unused warnings in prod builds Right: it might if -Wextra were enabled; as of now, it's not present in Kudu flags for the compiler. I added [[maybe_unused]] attribute since this code requires C++17 and newer: it should help to address the issue, if any. http://gerrit.cloudera.org:8080/#/c/24176/1/src/kudu/rpc/connection.cc@508 PS1, Line 508: auto t(OutboundTransfer::CreateForCallResponse(std::move(tmp_slices), std::move(cb))); > This will leak tmp_slices and cb if CreateForCallResponseThrows out of memo I'm not sure that's true and/or relevant. First, even if CreateForCallResponse throws, the whole process will crash since exceptions aren't handled at the upper level, at least in Kudu. Second, a small sample below shows there will not be a leak if a function throws before calling the constructor of the object that adopts the ownership of the objects passed as arguments-by-value. ----------- $ ./a.out A::A(10) A::A(0) A::~A(): i = 0 A::~A(): i = 10 xxx ------------ #include <exception> #include <iostream> #include <memory> #include <vector> using namespace std; class A { public: explicit A(int i) : i_(i) { cerr << "A::A(" << i_ << ")" << '\n'; } virtual ~A() { cerr << "A::~A(): i = " << i_ << '\n'; } private: int i_; }; class B { public: explicit B(unique_ptr<A> a, vector<A> va) : a_(std::move(a)), va_(std::move(va)) { cerr << "B::B(...)" << '\n'; } virtual ~B() { cerr << "B::~B()" << '\n'; } private: unique_ptr<A> a_; vector<A> va_; }; unique_ptr<B> f(unique_ptr<A> a, vector<A> va) { throw invalid_argument("xxx"); return unique_ptr<B>(new B(std::move(a), std::move(va))); } int main() { try { unique_ptr<A> a(make_unique<A>(10)); vector<A> va; va.emplace_back(0); auto b = f(std::move(a), std::move(va)); } catch (const std::exception& e) { cerr << e.what() << '\n'; } return 0; } http://gerrit.cloudera.org:8080/#/c/24176/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: http://gerrit.cloudera.org:8080/#/c/24176/2/src/kudu/rpc/connection.cc@699 PS2, Line 699: continue; Right: reading https://theboostcpplibraries.com/boost.intrusive should help addressing the confusion. > If the object needs to be explicitly deleted, I think it'd be clearer to > store a pointer in recycled_transfers and avoid the confusing '*' and '&'. I think that a separate changelist might be a great venue for implementing such sort of re-factoring. If it's OK with you, I'd like to keep the current arrangement for outbound_transfers_ elements as-is in this patch. The reasoning is simple: I don't want to mix different concerns in a single changelist, trying to focus on improving I/O efficiency. -- To view, visit http://gerrit.cloudera.org:8080/24176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf7ab105a851ef4d583efc2d1b33d57607810df0 Gerrit-Change-Number: 24176 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 09 Apr 2026 21:13:16 +0000 Gerrit-HasComments: Yes
