Alexey Serbin has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/24176 )
Change subject: [rpc] in-bulk memory recycling for
Connection::ProcessOutboundTransfers()
......................................................................
[rpc] in-bulk memory recycling for Connection::ProcessOutboundTransfers()
While troubleshooting RPC performance issues in a highly concurrent
workload, I noticed a pattern of lock contention in tcmalloc. Among
the captured stack traces, multiple reactor threads in a single pstack
snapshot often had stack traces similar to the one below.
For more context, the majority of RPCs had relatively large side-cars
and a single RPC connection often had a multitude of outgoing in-flight
transfers. The socket buffer size was around 128MB at the OS level.
It seems the issue manifests itself when many concurrent allocations
and deallocations go through the tcmalloc's central free list, while
the latter is guarded by a lock. There might be multiple reasons why
it happens: see [1]. Regardless of the underlying reasons, a reactor
thread is more efficient if performing as much I/O as possible at once
without the risk of waiting on a synchronization primitive and then
being de-scheduled off the CPU while there is still data ready to be
written into a socket whose buffer isn't full yet.
This patch is an attempt to reduce the described lock contention by
performing pending socket I/O for all the outgoing transfers first, and
deallocating the memory in-bulk after completing all the pending write
activity. In addition, it straightens memory ownership rules for the
OutboundTransfer::callbacks_ field and modernizes signatures of related
methods to use std::unique_ptr instead of raw pointers.
#0 sys_futex (... <tcmalloc::Static::pageheap_lock_>)
#1 base::internal::SpinLockDelay (...)
#2 base::internal::SpinLockDelay (...)
#3 SpinLock::SlowLock() ()
#4 tcmalloc::CentralFreeList::ReleaseToSpans(...) ()
#5 tcmalloc::CentralFreeList::ReleaseListToSpans(...) ()
#6 tcmalloc::CentralFreeList::InsertRange(...) ()
#7 tcmalloc::ThreadCache::ReleaseToCentralCache(...) ()
#8 tcmalloc::ThreadCache::ListTooLong(...) ()
#9 google::protobuf::internal::ArenaImpl::~ArenaImpl() ()
#10 google::protobuf::Arena::~Arena ()
#11 kudu::rpc::InboundCall::~InboundCall (...)
#12 std::default_delete<kudu::rpc::InboundCall>::operator()
#13 std::unique_ptr<kudu::rpc::InboundCall, ...>::~unique_ptr (...)
#14 kudu::rpc::ResponseTransferCallbacks::~ResponseTransferCallbacks
#15 kudu::rpc::ResponseTransferCallbacks::~ResponseTransferCallbacks (...)
#16 kudu::rpc::ResponseTransferCallbacks::NotifyTransferFinished (...)
#17 kudu::rpc::OutboundTransfer::SendBuffer (...)
#18 kudu::rpc::Connection::ProcessOutboundTransfers (...)
#19 kudu::rpc::Connection::QueueOutbound (...)
#20 kudu::rpc::QueueTransferTask::Run (...)
#21 kudu::rpc::ReactorThread::AsyncHandler (...)
#22 ev_invoke_pending ()
#23 kudu::rpc::ReactorThread::InvokePendingCb (...)
#24 ev_run ()
#25 ev::loop_ref::run (...)
#26 kudu::rpc::ReactorThread::RunThread (...)
I ran the newly added test with the paramters as below on 8 CPU core
machine under perf before and after this patch, 50 iterations each.
The idea was to oversubscribe on CPU resources and see how it runs.
mt-rpc-test --gtest_filter=*Sidecar* \
--mt_rpc_clients_num=12 \
--mt_rpc_iterations_num=32768 \
--mt_rpc_server_reactors_num=4'
before:
155,551.50 msec task-clock:HG # 7.020 CPUs utilized ( +- 0.28% )
2,297,600 context-switches:HG # 14.771 K/sec ( +- 1.44% )
243,684 cpu-migrations:HG # 1.567 K/sec ( +- 4.37% )
118,185 page-faults:HG # 759.781 /sec ( +- 0.16% )
22.159 +- 0.280 seconds time elapsed ( +- 1.26% )
after:
155,350.60 msec task-clock:HG # 7.197 CPUs utilized ( +- 0.21% )
2,296,021 context-switches:HG # 14.780 K/sec ( +- 1.43% )
226,160 cpu-migrations:HG # 1.456 K/sec ( +- 3.28% )
118,243 page-faults:HG # 761.136 /sec ( +- 0.15% )
21.586 +- 0.186 seconds time elapsed ( +- 0.86% )
The results of the RPC benchmark scenario run before and after
this patch shown no performance degradation:
rpc-bench --gtest_filter='*BenchmarkCalls'
[1] https://gperftools.github.io/gperftools/tcmalloc.html
Change-Id: Idf7ab105a851ef4d583efc2d1b33d57607810df0
Reviewed-on: http://gerrit.cloudera.org:8080/24176
Tested-by: Alexey Serbin <[email protected]>
Reviewed-by: Abhishek Chennaka <[email protected]>
Reviewed-by: Marton Greber <[email protected]>
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
5 files changed, 233 insertions(+), 56 deletions(-)
Approvals:
Alexey Serbin: Verified
Abhishek Chennaka: Looks good to me, approved
Marton Greber: Looks good to me, but someone else must approve
--
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: merged
Gerrit-Change-Id: Idf7ab105a851ef4d583efc2d1b33d57607810df0
Gerrit-Change-Number: 24176
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Abhishek Chennaka <[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: Marton Greber <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
