Skip to content
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

perf: optimize header list size calculations #750

Merged
merged 1 commit into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ jobs:
strategy:
matrix:
rust:
- nightly
- beta
- stable
steps:
Expand Down
23 changes: 18 additions & 5 deletions src/frame/headers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::fmt;
use std::io::Cursor;

type EncodeBuf<'a> = bytes::buf::Limit<&'a mut BytesMut>;

/// Header frame
///
/// This could be either a request or a response.
Expand Down Expand Up @@ -87,6 +88,9 @@ struct HeaderBlock {
/// The decoded header fields
fields: HeaderMap,

/// Precomputed size of all of our header fields, for perf reasons
field_size: usize,

/// Set to true if decoding went over the max header list size.
is_over_size: bool,

Expand Down Expand Up @@ -115,6 +119,7 @@ impl Headers {
stream_id,
stream_dep: None,
header_block: HeaderBlock {
field_size: calculate_headermap_size(&fields),
fields,
is_over_size: false,
pseudo,
Expand All @@ -131,6 +136,7 @@ impl Headers {
stream_id,
stream_dep: None,
header_block: HeaderBlock {
field_size: calculate_headermap_size(&fields),
fields,
is_over_size: false,
pseudo: Pseudo::default(),
Expand Down Expand Up @@ -196,6 +202,7 @@ impl Headers {
stream_dep,
header_block: HeaderBlock {
fields: HeaderMap::new(),
field_size: 0,
is_over_size: false,
pseudo: Pseudo::default(),
},
Expand Down Expand Up @@ -350,6 +357,7 @@ impl PushPromise {
PushPromise {
flags: PushPromiseFlag::default(),
header_block: HeaderBlock {
field_size: calculate_headermap_size(&fields),
fields,
is_over_size: false,
pseudo,
Expand Down Expand Up @@ -441,6 +449,7 @@ impl PushPromise {
flags,
header_block: HeaderBlock {
fields: HeaderMap::new(),
field_size: 0,
is_over_size: false,
pseudo: Pseudo::default(),
},
Expand Down Expand Up @@ -892,6 +901,8 @@ impl HeaderBlock {

headers_size += decoded_header_size(name.as_str().len(), value.len());
if headers_size < max_header_list_size {
self.field_size +=
decoded_header_size(name.as_str().len(), value.len());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that decoded_header_size won't exceed max_header_list_sized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is covered under other checks elsewhere

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm, that's exactly what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't know why I said that lol, it's literally checked right here.

self.fields.append(name, value);
} else if !self.is_over_size {
tracing::trace!("load_hpack; header list size over max");
Expand Down Expand Up @@ -958,14 +969,16 @@ impl HeaderBlock {
+ pseudo_size!(status)
+ pseudo_size!(authority)
+ pseudo_size!(path)
+ self
.fields
.iter()
.map(|(name, value)| decoded_header_size(name.as_str().len(), value.len()))
.sum::<usize>()
+ self.field_size
}
}

fn calculate_headermap_size(map: &HeaderMap) -> usize {
map.iter()
.map(|(name, value)| decoded_header_size(name.as_str().len(), value.len()))
.sum::<usize>()
}

fn decoded_header_size(name: usize, value: usize) -> usize {
name + value + 32
}
Expand Down
1 change: 1 addition & 0 deletions src/proto/streams/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ impl Store {
}
}

#[allow(clippy::blocks_in_conditions)]
pub(crate) fn for_each<F>(&mut self, mut f: F)
where
F: FnMut(Ptr),
Expand Down