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 2022/11/29 02:49:51 UTC
[GitHub] [trafficserver] maskit opened a new pull request, #9042: Eliminate a redundant data copy in H3 framer
maskit opened a new pull request, #9042:
URL: https://github.com/apache/trafficserver/pull/9042
Data copy was needed because of buffer type difference in HTTP/3 module. This PR removes the data copy (IOBufferBlock -> Regular buffer -> IOBufferBlock) by keeping the data in the original IOBufferBlock.
This change could land on master as well, but I didn't want to accidentally break HTTP/3 on master because this change is only tested with other changes on 10-Dev.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#discussion_r1070128044
##########
proxy/http3/Http3DataFramer.cc:
##########
@@ -37,7 +37,7 @@ Http3DataFramer::generate_frame()
Http3FrameUPtr frame = Http3FrameFactory::create_null_frame();
IOBufferReader *reader = this->_source_vio->get_reader();
- size_t payload_len = 128 * 1024;
+ size_t payload_len = 1024 * 1024;
Review Comment:
Depends on what you mean by send. It just try not to split data too much near the source. I don't remember the detail but the data chunk from cache has 1MB in size, IIRC.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on a diff in pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by GitBox <gi...@apache.org>.
bryancall commented on code in PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#discussion_r1049937402
##########
proxy/http3/Http3DataFramer.cc:
##########
@@ -37,7 +37,7 @@ Http3DataFramer::generate_frame()
Http3FrameUPtr frame = Http3FrameFactory::create_null_frame();
IOBufferReader *reader = this->_source_vio->get_reader();
- size_t payload_len = 128 * 1024;
+ size_t payload_len = 1024 * 1024;
Review Comment:
Does this mean we are going to send up to 1MB of data at a time?
##########
proxy/http3/Http3Frame.cc:
##########
@@ -160,18 +166,31 @@ Http3DataFrame::to_io_buffer_block() const
size_t n = 0;
size_t written = 0;
+ // Make a block for header
block = make_ptr<IOBufferBlock>(new_IOBufferBlock());
- block->alloc(iobuffer_size_to_index(HEADER_OVERHEAD + this->length(), BUFFER_SIZE_INDEX_32K));
+ block->alloc(iobuffer_size_to_index(HEADER_OVERHEAD, BUFFER_SIZE_INDEX_8K));
uint8_t *block_start = reinterpret_cast<uint8_t *>(block->start());
QUICVariableInt::encode(block_start, UINT64_MAX, n, static_cast<uint64_t>(this->_type));
written += n;
QUICVariableInt::encode(block_start + written, UINT64_MAX, n, this->_length);
written += n;
- memcpy(block_start + written, this->_payload, this->_payload_len);
- written += this->_payload_len;
-
block->fill(written);
+
+ // Append payload
Review Comment:
Should this be a IOBufferChain and use the write() method?
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1421423895
Flame graph before this patch:
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1421368540
I am still seeing a performance drop with this PR:
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1321358920
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] closed pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #9042: Eliminate a redundant data copy in H3 framer
URL: https://github.com/apache/trafficserver/pull/9042
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1401199721
Interesting. It might depend on test condition. I'll benchmark this again.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] closed pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9042: Eliminate a redundant data copy in H3 framer
URL: https://github.com/apache/trafficserver/pull/9042
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1539279908
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1330004946
It's not stale!!
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#discussion_r1070132908
##########
proxy/http3/Http3Frame.cc:
##########
@@ -160,18 +166,31 @@ Http3DataFrame::to_io_buffer_block() const
size_t n = 0;
size_t written = 0;
+ // Make a block for header
block = make_ptr<IOBufferBlock>(new_IOBufferBlock());
- block->alloc(iobuffer_size_to_index(HEADER_OVERHEAD + this->length(), BUFFER_SIZE_INDEX_32K));
+ block->alloc(iobuffer_size_to_index(HEADER_OVERHEAD, BUFFER_SIZE_INDEX_8K));
uint8_t *block_start = reinterpret_cast<uint8_t *>(block->start());
QUICVariableInt::encode(block_start, UINT64_MAX, n, static_cast<uint64_t>(this->_type));
written += n;
QUICVariableInt::encode(block_start + written, UINT64_MAX, n, this->_length);
written += n;
- memcpy(block_start + written, this->_payload, this->_payload_len);
- written += this->_payload_len;
-
block->fill(written);
+
+ // Append payload
Review Comment:
Maybe? Interface around IOBuffer is messy. I wanted to avoid memcpy and I did it in the most clear way.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9042: Eliminate a redundant data copy in H3 framer
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9042:
URL: https://github.com/apache/trafficserver/pull/9042#issuecomment-1401140282
This is odd, I am seeing worse performance when benchmarking this patch:
Without the patch:
```
finished in 7.23s, 69188.73 req/s, 78.12MB/s
requests: 500000 total, 500000 started, 500000 done, 500000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 500000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 564.58MB (592000000) total, 73.43MB (77000000) headers (space savings 35.56%), 488.28MB (512000000) data
UDP datagram: 695977 sent, 1500234 received
min max mean sd +/- sd
time for request: 100us 30.42ms 1.41ms 536us 72.82%
time for connect: 10.60ms 38.26ms 28.97ms 6.05ms 62.00%
time to 1st byte: 39.49ms 43.89ms 41.77ms 971us 60.00%
req/s : 692.24 752.59 704.10 16.78 84.00%
```
With the patch:
```
finished in 10.40s, 48079.78 req/s, 54.32MB/s
requests: 500000 total, 500000 started, 500000 done, 500000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 500000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 564.91MB (592355606) total, 73.77MB (77355606) headers (space savings 35.27%), 488.28MB (512000000) data
UDP datagram: 1026377 sent, 2500606 received
min max mean sd +/- sd
time for request: 152us 24.44ms 2.04ms 620us 72.30%
time for connect: 7.15ms 34.88ms 23.30ms 6.57ms 59.00%
time to 1st byte: 24.14ms 39.22ms 34.20ms 3.25ms 53.00%
req/s : 480.99 536.85 490.17 16.35 84.00%
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org