Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19498.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Pre-allocate the buffer based on the expected `Content-Length` with the Rust HTTP client.
42 changes: 41 additions & 1 deletion rust/src/http_client.rs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am a bit sceptical of that manual chunk aggregation though.

Fine with making it better in another PR, but we could:

Each of those steps are supposedly quite efficient: collecting the body in a list of buffers is done by moving references, aggregating that to a contiguous buffer pre-allocates the right size, and the conversion from bytes::Bytes to PyBytes is also done without copy.

Another thing we could make better, is that if we know that we're consuming it as a string, we may want to do the bytes->str conversion in Rust-land? Even one step further, what if we did the JSON parsing on the Rust side with something like pythonize?

EDIT: I've done some benchmarking: https://gitlab.element.io/quenting/pyo3-json-benchmark

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previous discussion on using http_body_util::Limited #18357 (comment)

Copy link
Copy Markdown
Contributor Author

@MadLittleMods MadLittleMods Feb 27, 2026

Choose a reason for hiding this comment

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

Sounds like a good improvement 👍 I've tackled the first part of this in #19510

Only weird thing I noticed in your description was:

and the conversion from bytes::Bytes to PyBytes is also done without copy.

As the docs actually say this:

When converting Bytes back into Python, this will do a copy, just like &[u8] and Vec<u8>.

-- https://docs.rs/pyo3/latest/pyo3/bytes/index.html

But that sounds just as equivalent to before ⏩


We can explore the JSON stuff further once we have some better traces to nail down whether that's the problem.

For example, if I recall correctly, that trace example in the PR description is from a single event (not big JSON parsing)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's still go for merging this PR even though #19510 supersedes it after

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use std::{collections::HashMap, future::Future, sync::OnceLock};

use anyhow::Context;
use futures::TryStreamExt;
use headers::HeaderMapExt;
use once_cell::sync::OnceCell;
use pyo3::{create_exception, exceptions::PyException, prelude::*};
use reqwest::RequestBuilder;
Expand Down Expand Up @@ -235,8 +236,47 @@ impl HttpClient {

let status = response.status();

// Find the expected `Content-Length` so we can pre-allocate the buffer
// necessary to read the response. It's expected that not every request will
// have a `Content-Length` header.
//
// `response.content_length()` does exist but the "value does not directly
// represents the value of the `Content-Length` header, but rather the size
// of the response’s body"
// (https://docs.rs/reqwest/latest/reqwest/struct.Response.html#method.content_length)
// and we want to avoid reading the entire body at this point because we
// purposely stream it below until the `response_limit`.
let content_length = {
let content_length = response
.headers()
.typed_get::<headers::ContentLength>()
// We need a `usize` for the `Vec::with_capacity(...)` usage below
.and_then(|content_length| content_length.0.try_into().ok());

// Sanity check that the request isn't too large from the information
// they told us (may be inaccurate so we also check below as we actually
// read the bytes)
if let Some(content_length_bytes) = content_length {
if content_length_bytes > response_limit {
Err(anyhow::anyhow!(
"Response size (defined by `Content-Length`) too large"
))?;
}
}

content_length
};

// Stream the response to avoid allocating a giant object on the server
// above our expected `response_limit`.
let mut stream = response.bytes_stream();
let mut buffer = Vec::new();
// Pre-allocate the buffer based on the expected `Content-Length`
let mut buffer = Vec::with_capacity(
content_length
// Default to pre-allocating nothing when the request doesn't have a
// `Content-Length` header
.unwrap_or(0),
);
while let Some(chunk) = stream.try_next().await.context("reading body")? {
if buffer.len() + chunk.len() > response_limit {
Err(anyhow::anyhow!("Response size too large"))?;
Expand Down
Loading