You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/12/20 22:02:01 UTC
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8895
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
KUDU-1865: Avoid some heap allocations in RPC paths
As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.
This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object
- Introduce a new interface RpcContext::RespondSuccess(const Message& message)
which allows callers to pass the response PB directly as argument instead
of using the preallocated response PB owned by RpcContext. This allows callers
to allocate the response PB on the stack, thus avoiding the need to use the heap.
- To complement the stack allocation of Response PB, we wrap the code which allocates
the Response PB in a new function GeneratedServiceIf::AllocResponsePB() which allows
derived classes to override it. If a service decides that all response PB will be
allocated on the stack, the overridden function can always return a nullptr.
Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
13 files changed, 161 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8895/1
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Sailesh Mukil, Todd Lipcon, Mostafa Mokhtar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8895
to look at the new patch set (#3).
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
KUDU-1865: Avoid some heap allocations in RPC paths
As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.
This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object
Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
11 files changed, 100 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8895/3
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 4:
Thanks for the review. We need to re-evaluate the need for it after IMPALA-5518 is fixed. It's unclear at this point whether this is manifesting as a side-effect of that bug.
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:05:23 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/result_tracker.cc@275
PS1, Line 275: ErrorStatusPB_RpcErrorCodePB err,
> warning: parameter 'err' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h@306
PS1, Line 306: GetTokenResponsePB* resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h@316
PS1, Line 316: void TestArgumentsInDiffPackage(const ReqDiffPackagePB *req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h@317
PS1, Line 317: RespDiffPackagePB *resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc_context.h
File src/kudu/rpc/rpc_context.h:
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc_context.h@219
PS1, Line 219: RpcContext(InboundCall *call);
> warning: single-argument constructors must be marked explicit to avoid unin
Done
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/service_if.h
File src/kudu/rpc/service_if.h:
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/service_if.h@113
PS1, Line 113: void Handle(InboundCall* incoming) override;
> warning: function 'kudu::rpc::GeneratedServiceIf::Handle' has a definition
Done
http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/service_if.h@120
PS1, Line 120: virtual google::protobuf::Message* AllocResponsePB(const RpcMethodInfo* method);
> warning: function 'kudu::rpc::GeneratedServiceIf::AllocResponsePB' has a de
Done
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 20 Dec 2017 22:35:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 4:
The code looks OK, but would be great to get some positive perf results. As I understand it, the tcmalloc issues with Impala are largely resolved after a bug fix, and this seems to increase complexity a bit. I can't see any significant perf difference using rpc-bench.
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jan 2018 00:40:19 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@16
PS2, Line 16: central cache list in TCMalloc and causes spin lock contention.
> would be good to accompany this with some benchmark results
I can look into rpc-bench. The perf infrastructure I have been using is down recently so I cannot run the typical TPC-H and TPC-DS benchmark at this point.
An anecdotal data point is a stress test which used to take 86 mins went down to about 57 mins but it's not quite scientific measurement.
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@21
PS2, Line 21: Introduce a new interface RpcContext::RespondSuccess(const Message& message)
: which allows callers to pass the response PB directly as argument instead
: of using the preallocated response PB owned by RpcContext. This allows callers
: to allocate the response PB on the stack, thus avoiding the need to use the heap.
> I'm not so keen on this API change here. It seems a bit messy to have two d
Yes, I am not too keen on having two APIs either. In fact, I wonder if there is a reason why all existing calls to RespondSuccess() cannot switch over to using the new interface and deprecate the old interface. The few RPCs I looked at seem to be easy to convert but there may be tricky ones which I didn't look at. This seems to make sense to me and we can get rid of the allocation for the response buffer in GeneratedServiceIf::Handle().
I haven't used 'Arena' before so would you mind explaining how it would help here. Does it allocate a large enough chunk to satisfy the allocations for request and other items so the malloc() call will be amortized ? So, in other words, heap allocations are still needed.
I will split the interface change out as a separate change.
http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/connection.cc@430
PS2, Line 430: scoped_refptr<Connection> conn_;
> is this required for correctness in this patch?
Yes. I ran into a case in rpc-test in which the dtor of InboundCall() recursively calls into the dtor of 'conn_' in which the 'inbound_call_pool_' resides.
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Dec 2017 06:32:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@21
PS2, Line 21: Introduce a new interface RpcContext::RespondSuccess(const Message& message)
: which allows callers to pass the response PB directly as argument instead
: of using the preallocated response PB owned by RpcContext. This allows callers
: to allocate the response PB on the stack, thus avoiding the need to use the heap.
> Yes, I am not too keen on having two APIs either. In fact, I wonder if ther
It'd also be nice if there is an object pool which can recycle the request and response buffer but it would have to be a per-thread pool or we may run into the lock contention when multiple service threads try to allocate from it.
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Dec 2017 06:43:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8895
to look at the new patch set (#2).
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
KUDU-1865: Avoid some heap allocations in RPC paths
As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.
This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object
- Introduce a new interface RpcContext::RespondSuccess(const Message& message)
which allows callers to pass the response PB directly as argument instead
of using the preallocated response PB owned by RpcContext. This allows callers
to allocate the response PB on the stack, thus avoiding the need to use the heap.
- To complement the stack allocation of Response PB, we wrap the code which allocates
the Response PB in a new function GeneratedServiceIf::AllocResponsePB() which allows
derived classes to override it. If a service decides that all response PB will be
allocated on the stack, the overridden function can always return a nullptr.
Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
13 files changed, 165 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8895/2
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/service_if.h
File src/kudu/rpc/service_if.h:
http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/service_if.h@118
PS2, Line 118: // Allocate a response PB for the given method. The memory returned is owned by the RpcContext.
: // May be overridden by derived class.
: virtual google::protobuf::Message* AllocResponsePB(const RpcMethodInfo* method_info);
Not entirely sure this is the most preferable approach. Open to suggestions.
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Dec 2017 00:18:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@16
PS2, Line 16: central cache list in TCMalloc and causes spin lock contention.
would be good to accompany this with some benchmark results
http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@21
PS2, Line 21: Introduce a new interface RpcContext::RespondSuccess(const Message& message)
: which allows callers to pass the response PB directly as argument instead
: of using the preallocated response PB owned by RpcContext. This allows callers
: to allocate the response PB on the stack, thus avoiding the need to use the heap.
I'm not so keen on this API change here. It seems a bit messy to have two different ways of responding.
Instead, what if each call used a single 'Arena' instance, and the response object in GeneratedServiceIf::Handle(...) allocated the response on that arena? The request and other such items could also be on that Arena?
At the least I think it's worth splitting this API-changing patch out from the part of the patch which is a pure transparent optimization
http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/connection.cc@430
PS2, Line 430: scoped_refptr<Connection> conn_;
is this required for correctness in this patch?
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Dec 2017 04:45:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Sailesh Mukil, Todd Lipcon, Mostafa Mokhtar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8895
to look at the new patch set (#4).
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
KUDU-1865: Avoid some heap allocations in RPC paths
As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.
This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object
Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
11 files changed, 100 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8895/4
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has abandoned this change. ( http://gerrit.cloudera.org:8080/8895 )
Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
......................................................................
Abandoned
Not needed after the fix of IMPALA-5518
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>