You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/10/20 03:38:10 UTC
[GitHub] [trafficserver] maskit opened a new pull request #7282: Reduce the number of write operation on H2
maskit opened a new pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282
H1ClientSession can simply pass large (1MB) buffers that come from Cache to `SSLWriteBuffer` (== `SSL_write`), because there's no framing on H1. On the other hand, H2ClientSession can't do the same, because data has to be split into DATA frames (frame headers have to be inserted). During the framing, small (16KB) buffers are made on each DATA frame and H2ClientSession passes those to `SSLWriteBuffers`. This means that we call `SSLWriteBuffer` 64 times more on H2.
This change reduces the number of write operation (`SSLWriteBuffer`) on H2 by increasing the buffer (block) size and storing multiple DATA frames into the single large buffer.
Here's benchmark result on my laptop:
`$ time h2load -n10000 -c10 -m10 https://localhost:8443/8m`
Before:
```
finished in 58.30s, 171.54 req/s, 1.34GB/s
requests: 10000 total, 10000 started, 10000 done, 10000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 10000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 78.17GB (83932534571) total, 101.64KB (104077) headers (space savings 96.71%), 78.13GB (83886080000) data
min max mean sd +/- sd
time for request: 18.00ms 5.53s 483.43ms 516.19ms 93.72%
time for connect: 2.92ms 8.21ms 5.14ms 1.68ms 70.00%
time to 1st byte: 44.94ms 164.22ms 116.20ms 33.34ms 70.00%
req/s : 17.15 29.08 20.61 3.75 80.00%
real 0m58.313s
user 0m40.274s
sys 0m17.108s
```
After:
```
finished in 50.54s, 197.86 req/s, 1.55GB/s
requests: 10000 total, 10000 started, 10000 done, 10000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 10000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 78.17GB (83932534054) total, 101.14KB (103564) headers (space savings 96.72%), 78.13GB (83886080000) data
min max mean sd +/- sd
time for request: 14.06ms 6.96s 416.36ms 559.53ms 95.08%
time for connect: 2.75ms 7.41ms 4.89ms 1.48ms 60.00%
time to 1st byte: 30.32ms 115.90ms 79.89ms 31.21ms 50.00%
req/s : 19.79 28.42 23.34 3.79 60.00%
real 0m50.556s
user 0m36.718s
sys 0m13.533s
```
For reference, this is H1 result on the same condition:
```
finished in 50.20s, 199.18 req/s, 1.56GB/s
requests: 10000 total, 10000 started, 10000 done, 10000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 10000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 78.13GB (83889917615) total, 3.10MB (3247615) headers (space savings 0.00%), 78.13GB (83886080000) data
min max mean sd +/- sd
time for request: 15.03ms 18.19s 416.14ms 803.15ms 98.16%
time for connect: 3.57ms 10.81ms 6.10ms 2.10ms 80.00%
time to 1st byte: 6.88ms 27.72ms 13.49ms 6.91ms 80.00%
req/s : 19.92 26.83 24.18 2.27 70.00%
real 0m50.226s
user 0m36.963s
sys 0m12.938s
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r509826156
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -299,18 +299,38 @@ Http2ClientSession::set_half_close_local_flag(bool flag)
}
int64_t
-Http2ClientSession::xmit(const Http2TxFrame &frame)
+Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush)
{
int64_t len = frame.write_to(this->write_buffer);
+ this->_pending_sending_data_size += len;
+
+ // Force flush for some cases
+ if (!flush) {
+ // Flush if we already use half of the buffer to avoid adding a new block to the chain.
Review comment:
This is a wild guess, but we can achieve the same performance by just bumping the buffer size? (without these pending data tracking and flush functions)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop merged pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
zwoop merged pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-713978351
I'm going to make the block size and threshold configurable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit edited a comment on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit edited a comment on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-712876007
openclose_h2 failed but I can't reproduce it locally.
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-714316653
Added two settings and the detail is documented, and made the default buffer size small a little (256KB) because 1MB may be too big for some people.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-718158510
Cherry-picked to v9.0.x branch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r509628551
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -299,18 +299,38 @@ Http2ClientSession::set_half_close_local_flag(bool flag)
}
int64_t
-Http2ClientSession::xmit(const Http2TxFrame &frame)
+Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush)
{
int64_t len = frame.write_to(this->write_buffer);
+ this->_pending_sending_data_size += len;
+
+ // Force flush for some cases
+ if (!flush) {
+ // Flush if we already use half of the buffer to avoid adding a new block to the chain.
Review comment:
But the write_reenable gets the writing underway, correct? So it does ensure that we don't buffer up all the writes until the End of Stream.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] vmamidi commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
vmamidi commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-713017842
> Nice! Should we get this in for 9.0.x ?
yes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
bryancall commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r511032683
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -3901,6 +3901,19 @@ HTTP/2 Configuration
Clients that send smaller window increments lower than this limit will be immediately disconnected with an error
code of ENHANCE_YOUR_CALM.
+.. ts:cv:: CONFIG proxy.config.http2.write_buffer_block_size_index INT 11
Review comment:
Doing an index size seems weird. I think it would be better to specify the buffer size and say we round up or down based on a power of 2. Look to see if there is another config option or storage.config that does something similar.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-712706105
Autest grabbed an old commit.
```
error: no such commit bccfe896b8fe9ad7ba9b71484f8e4351ca1b8cb8
```
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-713016768
Nice! Should we get this in for 9.0.x ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r508890586
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -299,18 +299,38 @@ Http2ClientSession::set_half_close_local_flag(bool flag)
}
int64_t
-Http2ClientSession::xmit(const Http2TxFrame &frame)
+Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush)
{
int64_t len = frame.write_to(this->write_buffer);
+ this->_pending_sending_data_size += len;
+
+ // Force flush for some cases
+ if (!flush) {
+ // Flush if we already use half of the buffer to avoid adding a new block to the chain.
Review comment:
Did you check this behavior? I bit doubt calling `flush()` prevents adding a new block. Because `write_reenable()` doesn't do actual write.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-716899941
@zwoop Let me know (or just merge) if you think it works fine on docs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-716257067
Addressed Bryan's concern. Time threshold is also added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-714651395
[approve ci freebsd]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-712876007
openclose_h2 failed but I can't reproduce locally.
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r509871680
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -299,18 +299,38 @@ Http2ClientSession::set_half_close_local_flag(bool flag)
}
int64_t
-Http2ClientSession::xmit(const Http2TxFrame &frame)
+Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush)
{
int64_t len = frame.write_to(this->write_buffer);
+ this->_pending_sending_data_size += len;
+
+ // Force flush for some cases
+ if (!flush) {
+ // Flush if we already use half of the buffer to avoid adding a new block to the chain.
Review comment:
That might be better than current, but I think it would call SSLWriteBuffer more frequently than the one with pending mechanism, because the buffered data would be sent regardless of the amount of the data. The pending mechanism ensure such small writes don't happen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#issuecomment-716214318
Lets address Bryan's concern, and then land this for 9.0.0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r508933688
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -299,18 +299,38 @@ Http2ClientSession::set_half_close_local_flag(bool flag)
}
int64_t
-Http2ClientSession::xmit(const Http2TxFrame &frame)
+Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush)
{
int64_t len = frame.write_to(this->write_buffer);
+ this->_pending_sending_data_size += len;
+
+ // Force flush for some cases
+ if (!flush) {
+ // Flush if we already use half of the buffer to avoid adding a new block to the chain.
Review comment:
You're right. There's no guarantee, and "flush" may be not a good name in that sense. Like the big frame case explained on next line, `write_buffer` may have multiple blocks, but it's just an unfortunate case (it always has been), so it doesn't affect the sending process.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7282: Reduce the number of write operation on H2
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7282:
URL: https://github.com/apache/trafficserver/pull/7282#discussion_r509792976
##########
File path: proxy/http2/Http2ClientSession.cc
##########
@@ -299,18 +299,38 @@ Http2ClientSession::set_half_close_local_flag(bool flag)
}
int64_t
-Http2ClientSession::xmit(const Http2TxFrame &frame)
+Http2ClientSession::xmit(const Http2TxFrame &frame, bool flush)
{
int64_t len = frame.write_to(this->write_buffer);
+ this->_pending_sending_data_size += len;
+
+ // Force flush for some cases
+ if (!flush) {
+ // Flush if we already use half of the buffer to avoid adding a new block to the chain.
Review comment:
Correct, and there is also receive window on H2 layer. Basically, the total size of the buffered data will not be bigger than window size. Strictly speaking, it can be bigger if we send H2 frames that are not under flow control, but the sizes are vary small compare to DATA frame.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org