Eliminate extra copies for data frames #5897
Conversation
shinrich
force-pushed
the
remove-extra-frame-copies
branch
from
5858a8a to
f115e14
Compare
|
I would prefer not to merge H2 related PRs until we do thorough testing. Erick Tapia and @meeramn are doing thorough testing of H2. |
|
Sure. I'm just sharing my intermediate results. |
There was a problem hiding this comment.
I agree with the copy to the uint8_t payload_buffer[buf_len] on the stack is just overhead. But as you pointed out, we may want another copy.
From my (limited) experiments, writing frame header and payload on the same buffer to avoid giving a small buffer to the SSL_write is much faster than the current code. Because 1 memcpy & 1 SSL_write looks cheaper than 2 SSL_write.
Block this for the performance testing as @vmamidi said.
|
@vmamidi What should we do with this? |
| */ | ||
| bool loadClassifiers(const String &args, bool blacklist = true); | ||
|
|
||
| bool _prefixToBeRemoved = false; /**< @brief instructs the prefix (i.e. host:port) not to added to the cache key */ |
There was a problem hiding this comment.
Should this file by in this PR?
|
|
||
| Http2Frame data(HTTP2_FRAME_TYPE_DATA, stream->get_id(), flags); | ||
| data.alloc(buffer_size_index[HTTP2_FRAME_TYPE_DATA]); | ||
| http2_write_data(payload_buffer, payload_length, data.write()); |
There was a problem hiding this comment.
This is the only place that uses http2_write_data(). Please remove the function definition too.
| } | ||
|
|
||
| void | ||
| set_payload_size(size_t length) |
There was a problem hiding this comment.
It's a bit unfortunate because this is just for DATA frame. How about changing finalize so that it can handle DATA frame specially?
I'm also ok with adding this function but in that case please add a comment that says this is just for DATA frame to prevent misuse.
|
#6337 covers this. |
6 participants
Trying to address performance concerns with H2. This PR removes two copies that were present in Http2ConnectionState::send_a_data_frame method. The first copy was to a stack allocated buffer called payload. The second copy was from the stack allocated buffer to an IOBlock variable allocated with the Http2Frame object.
We removed the stack buffer, and instead of copying data from the SM provided reader to another IOBlock, we assign the SM provided reader to the Http2Frame. When the Http2Frame::xmit method is called, the data from the SM provided reader is copied into the write buffer associated with the Http2ClientSession netvc. This copy should just copy over the IOBlocks since the data is moving from MIOBuffer to MIOBuffer.
Using the proxy.config.res_track_memory and fetching a 10MB file, the number of allocates for the Http2Frame::alloc were reduced from 644 to 3. For the non-data frames, we still use the original IOBlock scheme to sending the smaller frame payloads like Settings and Window updates.
We may still want to do the copy for the data buffers if we can include the Http2Frame in the buffer with the data. That would eliminate the extra small TLS records on the wire due to writing out the headers separately from the data payload. Will do some more experiments with that approach and put up another PR if that pans out.