-
Notifications
You must be signed in to change notification settings - Fork 869
Reduce H2 header allocations #13420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Reduce H2 header allocations #13420
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -201,7 +201,16 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const | |
| if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY); | ||
| field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) { | ||
| auto authority{field->value_get()}; | ||
| header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true); | ||
|
|
||
| // Match the reparse so url_print() stays byte-identical (legacy path | ||
| // unchanged); require full consumption, else a stray '/', '?' or '#' is dropped. | ||
| const char *astart = authority.data(); | ||
|
Comment on lines
+205
to
+207
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's still not quite clear to me why the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me like these are fixes to the HTTP/2 to HTTP/1.1 header conversion, which aren't directly part of the optimization itself. Is that right? |
||
| const char *aend = authority.data() + authority.length(); | ||
|
|
||
| if (url_parse_internet(header.m_heap, header.m_http->u.req.m_url_impl, &astart, aend, true, true) != ParseResult::DONE || | ||
| astart != aend) { | ||
| return ParseResult::ERROR; | ||
| } | ||
|
|
||
| if (!is_connect_method) { | ||
| MIMEField *host = header.field_find(static_cast<std::string_view>(MIME_FIELD_HOST)); | ||
|
|
@@ -229,14 +238,27 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const | |
| // :path | ||
| if (MIMEField *field = header.field_find(PSEUDO_HEADER_PATH); | ||
| field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) { | ||
| auto path{field->value_get()}; | ||
| auto path{field->value_get()}; | ||
| auto *url = header.m_http->u.req.m_url_impl; | ||
|
|
||
| // Match the reparse's m_query/m_fragment split so cache/remap see the same | ||
| // fields; set on delimiter presence even when the value is empty. | ||
| if (auto hpos = path.find('#'); hpos != std::string_view::npos) { | ||
| url->set_fragment(header.m_heap, path.substr(hpos + 1), true); | ||
| path = path.substr(0, hpos); | ||
| } | ||
|
|
||
| if (auto qpos = path.find('?'); qpos != std::string_view::npos) { | ||
| url->set_query(header.m_heap, path.substr(qpos + 1), true); | ||
| path = path.substr(0, qpos); | ||
| } | ||
|
|
||
| // cut first '/' if there, because `url_print()` add '/' before printing path | ||
| if (path.starts_with("/"sv)) { | ||
| // Match the reparse; url_print() re-adds a single leading '/'. | ||
| while (path.starts_with("/"sv)) { | ||
| path.remove_prefix(1); | ||
| } | ||
|
|
||
| header.m_http->u.req.m_url_impl->set_path(header.m_heap, path, true); | ||
| url->set_path(header.m_heap, path, true); | ||
|
|
||
| header.field_delete(field); | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -607,6 +607,7 @@ HttpSM::state_read_client_request_header(int event, void *data) | |
| case VC_EVENT_ACTIVE_TIMEOUT: | ||
| // The user agent is hosed. Close it & | ||
| // bail on the state machine | ||
| _pre_parsed_ua_request = nullptr; // the stream is going away; the borrow would dangle | ||
| vc_table.cleanup_entry(_ua.get_entry()); | ||
| _ua.set_entry(nullptr); | ||
| set_ua_abort(HttpTransact::ABORTED, event); | ||
|
|
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data) | |
| // tokenize header // | ||
| ///////////////////// | ||
|
|
||
| ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used, | ||
| _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing, | ||
| t_state.http_config_param->http_request_line_max_size, | ||
| t_state.http_config_param->http_hdr_field_max_size); | ||
| ParseResult state; | ||
|
|
||
| if (_pre_parsed_ua_request != nullptr) { | ||
| // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty. | ||
| if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) { | ||
| ATS_PROBE1(milestone_ua_first_read, sm_id); | ||
| milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime(); | ||
| } | ||
| t_state.hdr_info.client_request.copy(_pre_parsed_ua_request); | ||
| _pre_parsed_ua_request = nullptr; | ||
| bytes_used = t_state.hdr_info.client_request.length_get(); | ||
| state = ParseResult::DONE; | ||
|
JosiahWI marked this conversation as resolved.
|
||
| } else { | ||
| state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used, | ||
| _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing, | ||
| t_state.http_config_param->http_request_line_max_size, | ||
| t_state.http_config_param->http_hdr_field_max_size); | ||
| } | ||
|
Comment on lines
+633
to
+648
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validation should be applied at parse-time, not on the use of the cached value. The cached value should invariantly already be validated. |
||
|
|
||
| client_request_hdr_bytes += bytes_used; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -452,11 +452,22 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) | |
| if (stream->trailing_header_is_possible()) { | ||
| // Don't leak the header_blocks from the initial, non-trailing headers. | ||
| ats_free(stream->header_blocks); | ||
| stream->header_blocks = nullptr; | ||
|
JosiahWI marked this conversation as resolved.
|
||
| } | ||
| stream->header_blocks = static_cast<uint8_t *>(ats_malloc(header_block_fragment_length)); | ||
| frame.reader()->memcpy(stream->header_blocks, header_block_fragment_length, header_block_fragment_offset); | ||
|
|
||
| if (frame.header().flags & HTTP2_FLAGS_HEADERS_END_HEADERS) { | ||
| // Avoids the per-request malloc+memcpy+free of the header block. | ||
| bool const end_headers = frame.header().flags & HTTP2_FLAGS_HEADERS_END_HEADERS; | ||
| uint8_t const *inplace_block = nullptr; | ||
|
|
||
| if (end_headers && frame.reader()->block_read_avail() >= | ||
| static_cast<int64_t>(header_block_fragment_offset) + static_cast<int64_t>(header_block_fragment_length)) { | ||
| inplace_block = reinterpret_cast<uint8_t const *>(frame.reader()->start()) + header_block_fragment_offset; | ||
| } else { | ||
| stream->header_blocks = static_cast<uint8_t *>(ats_malloc(header_block_fragment_length)); | ||
| frame.reader()->memcpy(stream->header_blocks, header_block_fragment_length, header_block_fragment_offset); | ||
| } | ||
|
|
||
| if (end_headers) { | ||
| // NOTE: If there are END_HEADERS flag, decode stored Header Blocks. | ||
| if (!stream->change_state(HTTP2_FRAME_TYPE_HEADERS, frame.header().flags)) { | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR, | ||
|
|
@@ -475,8 +486,12 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) | |
| } else { | ||
| stream->mark_milestone(Http2StreamMilestone::START_DECODE_HEADERS); | ||
| } | ||
| Http2ErrorCode result = stream->decode_header_blocks(*this->local_hpack_handle, | ||
| this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE)); | ||
| Http2ErrorCode result = inplace_block ? | ||
| stream->decode_header_blocks(*this->local_hpack_handle, | ||
| this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE), | ||
| inplace_block, header_block_fragment_length) : | ||
| stream->decode_header_blocks(*this->local_hpack_handle, | ||
| this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE)); | ||
|
|
||
| // If this was an outbound connection and the state was already closed, just clear the | ||
| // headers after processing. We just processed the header blocks to keep the dynamic table in | ||
|
|
@@ -2472,9 +2487,11 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream) | |
| http2_convert_header_from_1_1_to_2(send_hdr); | ||
| } | ||
|
|
||
| uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case | ||
| ts::LocalBuffer local_buffer(buf_len); | ||
| uint8_t *buf = local_buffer.data(); | ||
| uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case | ||
| // Keep typical header sets off the heap; oversized ones spill to it. | ||
| static_assert(HdrHeap::DEFAULT_SIZE * 2 <= 8192, "keep HEADERS encode stack buffer within the event-thread stack budget"); | ||
| ts::LocalBuffer<uint8_t, HdrHeap::DEFAULT_SIZE * 2> local_buffer(buf_len); | ||
| uint8_t *buf = local_buffer.data(); | ||
|
Comment on lines
+2490
to
+2494
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do I understand correctly that you are increasing the buffer capacity here to increase the chance that typical response headers will fit in the buffer? |
||
|
|
||
| stream->mark_milestone(Http2StreamMilestone::START_ENCODE_HEADERS); | ||
| Http2ErrorCode result = http2_encode_header_blocks(send_hdr, buf, buf_len, &header_blocks_size, *(this->peer_hpack_handle), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -283,10 +283,16 @@ Http2Stream::main_event_handler(int event, void *edata) | |
|
|
||
| Http2ErrorCode | ||
| Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size) | ||
| { | ||
| return this->decode_header_blocks(hpack_handle, maximum_table_size, header_blocks, header_blocks_length); | ||
| } | ||
|
|
||
| Http2ErrorCode | ||
| Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size, const uint8_t *block, uint32_t block_len) | ||
| { | ||
| Http2ErrorCode error = | ||
| http2_decode_header_blocks(&_receive_header, (const uint8_t *)header_blocks, header_blocks_length, nullptr, hpack_handle, | ||
| _trailing_header_is_possible, maximum_table_size, this->is_outbound_connection()); | ||
| http2_decode_header_blocks(&_receive_header, block, block_len, nullptr, hpack_handle, _trailing_header_is_possible, | ||
| maximum_table_size, this->is_outbound_connection()); | ||
|
Comment on lines
+286
to
+295
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for adding the 4-argument overload? |
||
| if (error != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) { | ||
| Http2StreamDebug("Error decoding header blocks: %u", static_cast<uint32_t>(error)); | ||
| } | ||
|
|
@@ -303,10 +309,13 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */) | |
|
|
||
| // Convert header to HTTP/1.1 format. Trailing headers need no conversion | ||
| // because they, by definition, do not contain pseudo headers. | ||
| bool conversion_ok = true; | ||
|
|
||
| if (this->trailing_header_is_possible()) { | ||
| Http2StreamDebug("trailing header: Skipping send_headers initialization."); | ||
| } else { | ||
| if (http2_convert_header_from_2_to_1_1(&_receive_header) == ParseResult::ERROR) { | ||
| conversion_ok = false; | ||
| Http2StreamDebug("Error converting HTTP/2 headers to HTTP/1.1."); | ||
| if (_receive_header.type_get() == HTTPType::REQUEST) { | ||
| // There's no way to cause Bad Request directly at this time. | ||
|
|
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */) | |
| this->_http_sm_id = this->_sm->sm_id; | ||
| } | ||
|
|
||
| // The fast path skips parse_req, so re-apply strict_uri_parsing on the target. | ||
| // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST. | ||
| auto uri_ok = [&]() { | ||
| int const level = this->_sm->t_state.http_config_param->strict_uri_parsing; | ||
|
|
||
| return level == 0 || | ||
| (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) && | ||
| url_is_uri_compliant(level, _receive_header.fragment_get())); | ||
| }; | ||
|
|
||
| // The conversion_ok gate keeps a \xffVOID method on the serialize path, where parse_req 400s it. | ||
| if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() && | ||
| _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok()) { | ||
| this->_sm->set_pre_parsed_ua_request(&_receive_header); | ||
| if (this->receive_end_stream) { | ||
| this->read_vio.nbytes = this->data_length; | ||
| this->read_vio.ndone = this->read_vio.nbytes; | ||
| this->signal_read_event(VC_EVENT_READ_COMPLETE); | ||
| } else { | ||
| this->has_body = true; | ||
| this->signal_read_event(VC_EVENT_READ_READY); | ||
| } | ||
| // Delivery is synchronous (stream mutex): the borrow is copied, or the txn | ||
| // finished and _sm is null. A still-pending borrow is unreachable; recover anyway. | ||
| if (this->_sm == nullptr || !this->_sm->has_pending_pre_parsed_ua_request()) { | ||
| return; | ||
| } | ||
| ink_assert(!"pre-parsed handoff was deferred"); | ||
| this->_sm->set_pre_parsed_ua_request(nullptr); | ||
| } | ||
|
Comment on lines
+360
to
+367
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one needs to be checked. |
||
|
|
||
| // Write header to a buffer. Borrowing logic from HttpSM::write_header_into_buffer. | ||
| // Seems like a function like this ought to be in HTTPHdr directly | ||
| int bufindex; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.