[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16209 ) Change subject: tls_socket: avoid cork/uncork dance for small writevs .. tls_socket: avoid cork/uncork dance for small writevs Prior to this patch, TlsSocket emulated Writev() by corking the socket, calling write() once per buffer, and then uncorking. The system call overhead here is pretty substantial. For short writes, it's much cheaper to pay the cost of memcpy into a contiguous buffer rather than doing the complex code path. I benchmarked this using a YCSB workload C (random read) workload in which I found that the server was reactor bound (YCSB is a single client so all traffic goes over a single connection). The read throughput improved from around 44k reads/second to around 52k reads/second (18% improvement) Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Reviewed-on: http://gerrit.cloudera.org:8080/16209 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/security/tls_context.cc M src/kudu/security/tls_socket.cc M src/kudu/security/tls_socket.h 3 files changed, 62 insertions(+), 4 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16209 ) Change subject: tls_socket: avoid cork/uncork dance for small writevs .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Jul 2020 02:20:10 + Gerrit-HasComments: No
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16209 to look at the new patch set (#2). Change subject: tls_socket: avoid cork/uncork dance for small writevs .. tls_socket: avoid cork/uncork dance for small writevs Prior to this patch, TlsSocket emulated Writev() by corking the socket, calling write() once per buffer, and then uncorking. The system call overhead here is pretty substantial. For short writes, it's much cheaper to pay the cost of memcpy into a contiguous buffer rather than doing the complex code path. I benchmarked this using a YCSB workload C (random read) workload in which I found that the server was reactor bound (YCSB is a single client so all traffic goes over a single connection). The read throughput improved from around 44k reads/second to around 52k reads/second (18% improvement) Change-Id: Ic6abf790a38878b41862655e643782a7624b763f --- M src/kudu/security/tls_context.cc M src/kudu/security/tls_socket.cc M src/kudu/security/tls_socket.h 3 files changed, 62 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/16209/2 -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16209 ) Change subject: tls_socket: avoid cork/uncork dance for small writevs .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.h File src/kudu/security/tls_socket.h: http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.h@64 PS1, Line 64: // Socket-local buffer used by Writev(). : faststring buf_; > This variable isn't used across Write() calls or such. Using a local variab it's used across Write calls in that we don't want to pay the malloc/free cost in each call http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@127 PS1, Line 127: int > nit: size_t to match the type of the total_size variable? I benchmarked the syscall overhead for setsockopt and redid the math here. It does work out that a bit larger of a buffer is actually beneficial. http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@134 PS1, Line 134: // TODO(todd) why int32 in once place and int64 in the other? > Write() call does take int32_t. Is this TODO still relevant? that's the issue -- Write takes int32 but Writev takes int64, so we need this local int32_t temporary here, which is kinda weird http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@138 PS1, Line 138: return s; > Should this be I don't think it's necessary since the Write() call itself already handles returning OK in that case. http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@144 PS1, Line 144: if (use_cork_) > While you are here optimizing the number of syscalls and the transfer of sm yep, thanks -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 23 Jul 2020 23:31:38 + Gerrit-HasComments: Yes
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16209 )
Change subject: tls_socket: avoid cork/uncork dance for small writevs
..
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@127
PS1, Line 127: 4096
Any specific criteria to set the limit to 4K? If yes, could you add a comment
explaining the choice?
BTW, would 8K or something higher be a better choice here?
http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@127
PS1, Line 127: int
nit: size_t to match the type of the total_size variable?
http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@138
PS1, Line 138: return s;
Should this be
if (*nwritten > 0 && Socket::IsTemporarySocketError(s.posix_code())) {
return Status::OK();
}
return s;
as at line 165 ?
http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@144
PS1, Line 144: if (use_cork_)
While you are here optimizing the number of syscalls and the transfer of small
amount of data, maybe change this condition here and at line 159 to be
if (use_cork_ && iov_len > 1)
?
--
To view, visit http://gerrit.cloudera.org:8080/16209
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f
Gerrit-Change-Number: 16209
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 Jul 2020 01:33:14 +
Gerrit-HasComments: Yes
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16209 ) Change subject: tls_socket: avoid cork/uncork dance for small writevs .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.h File src/kudu/security/tls_socket.h: http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.h@64 PS1, Line 64: // Socket-local buffer used by Writev(). : faststring buf_; This variable isn't used across Write() calls or such. Using a local variable should be fine. -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 17 Jul 2020 00:31:27 + Gerrit-HasComments: Yes
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16209 ) Change subject: tls_socket: avoid cork/uncork dance for small writevs .. Patch Set 1: (2 comments) The test failures look relevant. http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@134 PS1, Line 134: // TODO(todd) why int32 in once place and int64 in the other? Write() call does take int32_t. Is this TODO still relevant? http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@135 PS1, Line 135: n nit: Can this be named bytes_written instead just like below? -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 17 Jul 2020 00:22:46 + Gerrit-HasComments: Yes
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16209 ) Change subject: tls_socket: avoid cork/uncork dance for small writevs .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: http://gerrit.cloudera.org:8080/#/c/16209/1/src/kudu/security/tls_socket.cc@134 PS1, Line 134: // TODO(todd) why int32 in once place and int64 in the other? Now I'm curious, did you mean to add a comment here explaining this? Or is this something that makes it perform better but you're yet to figure out why? -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 17 Jul 2020 00:17:47 + Gerrit-HasComments: Yes
[kudu-CR] tls socket: avoid cork/uncork dance for small writevs
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/16209 to review the following change. Change subject: tls_socket: avoid cork/uncork dance for small writevs .. tls_socket: avoid cork/uncork dance for small writevs Prior to this patch, TlsSocket emulated Writev() by corking the socket, calling write() once per buffer, and then uncorking. The system call overhead here is pretty substantial. For short writes, it's much cheaper to pay the cost of memcpy into a contiguous buffer rather than doing the complex code path. I benchmarked this using a YCSB workload C (random read) workload in which I found that the server was reactor bound (YCSB is a single client so all traffic goes over a single connection). The read throughput improved from around 44k reads/second to around 52k reads/second (18% improvement) Change-Id: Ic6abf790a38878b41862655e643782a7624b763f --- M src/kudu/security/tls_socket.cc M src/kudu/security/tls_socket.h 2 files changed, 45 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/16209/1 -- To view, visit http://gerrit.cloudera.org:8080/16209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic6abf790a38878b41862655e643782a7624b763f Gerrit-Change-Number: 16209 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin
