Avoid unnecessary copy in HttpDecoder if entire payload is present. Move parsing logic from FinishParsing() to ParseEntirePayload() for CANCEL_PUSH, SETTINGS, GOAWAY, MAX_PUSH_ID, PRIORITY_UPDATE, PRIORITY_UPDATE_REQUEST_STREAM, and ACCEPT_CH frame types. Skip FinishParsing() for these frames except when payload is empty, when ReadFramePayload() is skipped. Rename BufferFramePayload() to BufferOrParsePayload(), only buffer if necessary, and call ParseEntirePayload() when possible. Pass QuicDataReader to FinishParsing() to make it convenient to pass it on to BufferOrParsePayload() where nothing is read from it. PiperOrigin-RevId: 365791256 Change-Id: I3fa150e2b886f024de01f85a6b25e5953a8cb0ec
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index 02b563d..6a5eee0 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -106,7 +106,7 @@ continue_processing = ReadFramePayload(&reader); break; case STATE_FINISH_PARSING: - continue_processing = FinishParsing(); + continue_processing = FinishParsing(&reader); break; case STATE_ERROR: break; @@ -282,15 +282,11 @@ break; } case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::SETTINGS): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): { @@ -359,33 +355,23 @@ break; } case static_cast<uint64_t>(HttpFrameType::GOAWAY): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); + continue_processing = BufferOrParsePayload(reader); break; } default: { @@ -394,14 +380,15 @@ } } - if (remaining_frame_length_ == 0) { + // BufferOrParsePayload() may have advanced |state_|. + if (state_ == STATE_READING_FRAME_PAYLOAD && remaining_frame_length_ == 0) { state_ = STATE_FINISH_PARSING; } return continue_processing; } -bool HttpDecoder::FinishParsing() { +bool HttpDecoder::FinishParsing(QuicDataReader* reader) { QUICHE_DCHECK_EQ(0u, remaining_frame_length_); bool continue_processing = true; @@ -416,34 +403,15 @@ break; } case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - CancelPushFrame frame; - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - if (!reader.ReadVarInt62(&frame.push_id)) { - RaiseError(QUIC_HTTP_FRAME_ERROR, - "Unable to read CANCEL_PUSH push_id."); - return false; - } - if (!reader.IsDoneReading()) { - RaiseError(QUIC_HTTP_FRAME_ERROR, - "Superfluous data in CANCEL_PUSH frame."); - return false; - } - continue_processing = visitor_->OnCancelPushFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::SETTINGS): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - SettingsFrame frame; - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - if (!ParseSettingsFrame(&reader, &frame)) { - return false; - } - continue_processing = visitor_->OnSettingsFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): { @@ -451,75 +419,33 @@ break; } case static_cast<uint64_t>(HttpFrameType::GOAWAY): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - GoAwayFrame frame; - if (!reader.ReadVarInt62(&frame.id)) { - RaiseError(QUIC_HTTP_FRAME_ERROR, "Unable to read GOAWAY ID."); - return false; - } - if (!reader.IsDoneReading()) { - RaiseError(QUIC_HTTP_FRAME_ERROR, "Superfluous data in GOAWAY frame."); - return false; - } - continue_processing = visitor_->OnGoAwayFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - MaxPushIdFrame frame; - if (!reader.ReadVarInt62(&frame.push_id)) { - RaiseError(QUIC_HTTP_FRAME_ERROR, - "Unable to read MAX_PUSH_ID push_id."); - return false; - } - if (!reader.IsDoneReading()) { - RaiseError(QUIC_HTTP_FRAME_ERROR, - "Superfluous data in MAX_PUSH_ID frame."); - return false; - } - continue_processing = visitor_->OnMaxPushIdFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - PriorityUpdateFrame frame; - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - if (!ParsePriorityUpdateFrame(&reader, &frame)) { - return false; - } - continue_processing = visitor_->OnPriorityUpdateFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - PriorityUpdateFrame frame; - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - if (!ParseNewPriorityUpdateFrame(&reader, &frame)) { - return false; - } - continue_processing = visitor_->OnPriorityUpdateFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - AcceptChFrame frame; - QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size()); - QuicDataReader reader(buffer_); - if (!ParseAcceptChFrame(&reader, &frame)) { - return false; - } - continue_processing = visitor_->OnAcceptChFrame(frame); + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); break; } default: @@ -557,23 +483,129 @@ } } -void HttpDecoder::BufferFramePayload(QuicDataReader* reader) { - if (current_frame_length_ == remaining_frame_length_) { +bool HttpDecoder::BufferOrParsePayload(QuicDataReader* reader) { + QUICHE_DCHECK_EQ(current_frame_length_, + buffer_.size() + remaining_frame_length_); + + bool continue_processing = true; + + if (buffer_.empty() && reader->BytesRemaining() >= current_frame_length_) { + // |*reader| contains entire payload, which might be empty. + remaining_frame_length_ = 0; + QuicDataReader current_payload_reader(reader->PeekRemainingPayload().data(), + current_frame_length_); + continue_processing = ParseEntirePayload(¤t_payload_reader); + reader->Seek(current_frame_length_); + } else { + if (buffer_.empty()) { + buffer_.reserve(current_frame_length_); + } + + // Buffer as much of the payload as |*reader| contains. + QuicByteCount bytes_to_read = std::min<QuicByteCount>( + remaining_frame_length_, reader->BytesRemaining()); + absl::StrAppend(&buffer_, reader->PeekRemainingPayload().substr( + /* pos = */ 0, bytes_to_read)); + reader->Seek(bytes_to_read); + remaining_frame_length_ -= bytes_to_read; + + QUICHE_DCHECK_EQ(current_frame_length_, + buffer_.size() + remaining_frame_length_); + + if (remaining_frame_length_ > 0) { + QUICHE_DCHECK(reader->IsDoneReading()); + return true; + } + + QuicDataReader buffer_reader(buffer_); + continue_processing = ParseEntirePayload(&buffer_reader); buffer_.clear(); - buffer_.reserve(current_frame_length_); } - QUICHE_DCHECK_EQ(current_frame_length_ - remaining_frame_length_, - buffer_.size()); - QuicByteCount bytes_to_read = std::min<QuicByteCount>( - remaining_frame_length_, reader->BytesRemaining()); - absl::StrAppend(&buffer_, reader->PeekRemainingPayload().substr( - /* pos = */ 0, bytes_to_read)); - reader->Seek(bytes_to_read); + current_length_field_length_ = 0; + current_type_field_length_ = 0; + state_ = STATE_READING_FRAME_TYPE; + return continue_processing; +} - remaining_frame_length_ -= bytes_to_read; - QUICHE_DCHECK_EQ(current_frame_length_ - remaining_frame_length_, - buffer_.size()); +bool HttpDecoder::ParseEntirePayload(QuicDataReader* reader) { + QUICHE_DCHECK_EQ(current_frame_length_, reader->BytesRemaining()); + QUICHE_DCHECK_EQ(0u, remaining_frame_length_); + + switch (current_frame_type_) { + case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): { + CancelPushFrame frame; + if (!reader->ReadVarInt62(&frame.push_id)) { + RaiseError(QUIC_HTTP_FRAME_ERROR, + "Unable to read CANCEL_PUSH push_id."); + return false; + } + if (!reader->IsDoneReading()) { + RaiseError(QUIC_HTTP_FRAME_ERROR, + "Superfluous data in CANCEL_PUSH frame."); + return false; + } + return visitor_->OnCancelPushFrame(frame); + } + case static_cast<uint64_t>(HttpFrameType::SETTINGS): { + SettingsFrame frame; + if (!ParseSettingsFrame(reader, &frame)) { + return false; + } + return visitor_->OnSettingsFrame(frame); + } + case static_cast<uint64_t>(HttpFrameType::GOAWAY): { + GoAwayFrame frame; + if (!reader->ReadVarInt62(&frame.id)) { + RaiseError(QUIC_HTTP_FRAME_ERROR, "Unable to read GOAWAY ID."); + return false; + } + if (!reader->IsDoneReading()) { + RaiseError(QUIC_HTTP_FRAME_ERROR, "Superfluous data in GOAWAY frame."); + return false; + } + return visitor_->OnGoAwayFrame(frame); + } + case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID): { + MaxPushIdFrame frame; + if (!reader->ReadVarInt62(&frame.push_id)) { + RaiseError(QUIC_HTTP_FRAME_ERROR, + "Unable to read MAX_PUSH_ID push_id."); + return false; + } + if (!reader->IsDoneReading()) { + RaiseError(QUIC_HTTP_FRAME_ERROR, + "Superfluous data in MAX_PUSH_ID frame."); + return false; + } + return visitor_->OnMaxPushIdFrame(frame); + } + case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE): { + PriorityUpdateFrame frame; + if (!ParsePriorityUpdateFrame(reader, &frame)) { + return false; + } + return visitor_->OnPriorityUpdateFrame(frame); + } + case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM): { + PriorityUpdateFrame frame; + if (!ParseNewPriorityUpdateFrame(reader, &frame)) { + return false; + } + return visitor_->OnPriorityUpdateFrame(frame); + } + case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { + AcceptChFrame frame; + if (!ParseAcceptChFrame(reader, &frame)) { + return false; + } + return visitor_->OnAcceptChFrame(frame); + } + default: + // Only above frame types are parsed by ParseEntirePayload(). + QUICHE_NOTREACHED(); + return false; + } } void HttpDecoder::BufferFrameLength(QuicDataReader* reader) {
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h index fe955c3..30ae932 100644 --- a/quic/core/http/http_decoder.h +++ b/quic/core/http/http_decoder.h
@@ -175,14 +175,17 @@ // if there are any errors. Returns whether processing should continue. bool ReadFrameLength(QuicDataReader* reader); - // Reads the payload of the current frame from |reader| and processes it, - // possibly buffering the data or invoking the visitor. Returns whether - // processing should continue. + // Depending on the frame type, reads and processes the payload of the current + // frame from |reader| and calls visitor methods, or calls + // BufferOrParsePayload(). Returns whether processing should continue. bool ReadFramePayload(QuicDataReader* reader); - // Optionally parses buffered data; calls visitor method to signal that frame - // had been parsed completely. Returns whether processing should continue. - bool FinishParsing(); + // For frame types parsed by BufferOrParsePayload(), this method is only + // called if frame payload is empty, at it calls BufferOrParsePayload(). For + // other frame types, this method directly calls visitor methods to signal + // that frame had been parsed completely. Returns whether processing should + // continue. + bool FinishParsing(QuicDataReader* reader); // Read payload of unknown frame from |reader| and call // Visitor::OnUnknownFramePayload(). Returns true decoding should continue, @@ -192,8 +195,16 @@ // Discards any remaining frame payload from |reader|. void DiscardFramePayload(QuicDataReader* reader); - // Buffers any remaining frame payload from |reader| into |buffer_|. - void BufferFramePayload(QuicDataReader* reader); + // Buffers any remaining frame payload from |*reader| into |buffer_| if + // necessary. Parses the frame payload if complete. Parses out of |*reader| + // without unnecessary copy if |*reader| has entire payload. + // Returns whether processing should continue. + bool BufferOrParsePayload(QuicDataReader* reader); + + // Parses the entire payload of certain kinds of frames that are parsed in a + // single pass. |reader| must have at least |current_frame_length_| bytes. + // Returns whether processing should continue. + bool ParseEntirePayload(QuicDataReader* reader); // Buffers any remaining frame length field from |reader| into // |length_buffer_|.