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/07/20 05:34:39 UTC
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7471
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
KUDU-1865: Avoid heap allocation for payload slices
As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.
Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
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/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 56 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/7471/1
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7471/3/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:
Line 185: *(slice_iter++) = Slice(response_hdr_buf_);
nit: the () here are not necessary, are they?
Line 187: for (auto& sidecar : outbound_sidecars_) *(slice_iter++) = sidecar->AsSlice();
style: can you split this back onto multiple lines with braces? we rarely use single-line loops unless they are totally trivial like empty bodies
http://gerrit.cloudera.org:8080/#/c/7471/3/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
Line 117: for (auto& sidecar : sidecars_) *(slice_iter++) = sidecar->AsSlice();
same
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7471
to look at the new patch set (#4).
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
KUDU-1865: Avoid heap allocation for payload slices
As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.
Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
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/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 59 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/7471/4
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7471/3/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:
Line 185: (*slices)[1] = Slice(response_msg_buf_);
> nit: the () here are not necessary, are they?
Done
Line 187: (*slices)[i+2] = outbound_sidecars_[i]->AsSlice();
> style: can you split this back onto multiple lines with braces? we rarely u
Done
http://gerrit.cloudera.org:8080/#/c/7471/3/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
Line 117: (*slices)[i+2] = sidecars_[i]->AsSlice();
> same
Done
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7471
to look at the new patch set (#2).
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
KUDU-1865: Avoid heap allocation for payload slices
As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.
Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
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/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 55 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/7471/2
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
KUDU-1865: Avoid heap allocation for payload slices
As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.
Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Reviewed-on: http://gerrit.cloudera.org:8080/7471
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
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/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 59 insertions(+), 48 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7471/1/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:
PS1, Line 125: Slice slices[TransferLimits::kMaxPayloadSlices]
I think using 'std::array<Slice, kMaxPayloadSlices>* slices' would be a bit more idiomatic here and elsewhere in the patch. The "fixed size array parameter" is pretty rarely used in C++ in my experience
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7471/1/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:
PS1, Line 125: Slice slices[TransferLimits::kMaxPayloadSlices]
> I think using 'std::array<Slice, kMaxPayloadSlices>* slices' would be a bit
Done
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7471
to look at the new patch set (#3).
Change subject: KUDU-1865: Avoid heap allocation for payload slices
......................................................................
KUDU-1865: Avoid heap allocation for payload slices
As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.
Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
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/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 56 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/7471/3
--
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>