Skip to content

perf: remove redundant allocations and copies from the request and response hot paths - #538

Merged
barjin merged 13 commits into
masterfrom
perf/hot-path-allocations
Sep 7, 2026
Merged

perf: remove redundant allocations and copies from the request and response hot paths#538
barjin merged 13 commits into
masterfrom
perf/hot-path-allocations

Conversation

@barjin

@barjin barjin commented Sep 2, 2026

Copy link
Copy Markdown
Member

Cuts per-request work across the Rust core and both bindings: the browser fingerprint is no longer cloned twice per request during header assembly, response bodies are no longer copied repeatedly in the Python bindings, the HTML prescan is skipped for responses that cannot contain a meta tag, and the Node JS wrapper stops marshalling the response headers across the napi boundary twice.

Header assembly cloned the whole BrowserFingerprint twice per request (once into
the builder, once when the builder was cloned into HttpHeaders), then serialized
the merged headers into a Vec<(String, String)> only for the send path to parse
them back into a HeaderMap through yet another builder round-trip.

The builder now borrows the fingerprint and the header slices and produces the
HeaderMap directly. The unused host/https builder fields are dropped along with
the per-request host allocation they required.
The hostname was allocated twice per request and the full URL was stringified
eagerly even though only the debug logs and the error context need it. The
method also went through a Method -> String -> Method round-trip between
build_request and send.

ImpitRequest now carries a Method, the URL and headers are moved into the
prepared request instead of being cloned, and the H3 engine takes &str.
Each prefix check allocated two Vecs just to compare a handful of bytes.
The headers map was cloned on entry and then cloned again entry-by-entry while
being turned into the RequestOptions vec, even though the binding already owns
it.
Reading a response copied the whole body three times: once out of the reqwest
Bytes into a Vec, once more on every content/text access, and finally into the
Python object. Text bodies were copied a fourth time by caching a clone of the
decoded string.

The body is now kept as Bytes and the getters borrow it, so only the copy into
the Python object remains. The bytes iterators hand out PyBytes directly for
the same reason.
Decoding a response without a charset in its Content-Type built a lol_html
rewriter - selector parsing included - and pushed the first kilobyte through
the HTML parser, even for JSON or binary payloads that cannot possibly contain
a meta tag. Both prescan handlers need a `<meta` start tag, so a scan of the
same prefix decides upfront whether the parser is needed at all, and the two
selectors are parsed once instead of per response.

Measured on a release build: 6.1us -> 1.5us for a JSON body, 32.7us -> 28.0us
for an HTML one.
fetch cloned the per-request header vec (and re-borrowed the init four times)
even though the init is dropped right after. Destructuring it once moves the
fields out. The response header vec is also preallocated.
The response headers were marshalled across the napi boundary twice - once for
cookie handling and again inside #wrapResponse - and each crossing costs about
0.7us per header. #wrapResponse now reuses the Headers object the caller
already built.

text() also went through bytes(), which adds a promise hop and a Uint8Array
just to hand the bytes back to decodeBuffer; it now decodes the ArrayBuffer
directly, the way clone() already did. Requests without an AbortSignal no
longer allocate an abort promise and race it, requests without a cookie jar no
longer copy and case-fold the header list, and the TextEncoder and redirect
status lookups are hoisted out of the per-call path.

Measured against a local server with 21 response headers, release build:
fetch + text drops from ~900us to ~720us.
@github-actions github-actions Bot added this to the 148th sprint - Tooling team milestone Sep 2, 2026
@github-actions github-actions Bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Sep 2, 2026

barjin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

The rustfmt CI check is currently failing on this branch (a plain cargo fmt run should fix it). Also note impit::request::ImpitRequest is a pub struct whose headers/method field types change from Vec<(String, String)>/String to HeaderMap/Method, which is source-breaking for any external Rust code constructing it directly.


Generated by Claude Code

The binding tests all point at a single httpbin instance that rejects anything
over 70 requests per second. One test run makes 190 httpbin requests over
15-30s, so at roughly 6-12 req/s per job the 17 jobs the matrix used to start
at once overran the limit and the suite failed on whichever assertions happened
to receive a rate-limit body - a different random set every run.

max-parallel caps each matrix at one job, so at most five run concurrently.

barjin commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

impit::request::ImpitRequest is still a pub struct whose headers/method fields change type from Vec<(String, String)>/String to HeaderMap/Method, which is source-breaking for any external Rust code constructing it directly (no version bump visible in this diff). No other functional issues found in the header-merging, response-buffering, or HTML-prescan changes.

// breaks: ImpitRequest.headers/method are no longer String-based
let req = impit::request::ImpitRequest { url, body: Default::default(), headers: vec[], method: "GET".into() };

Generated by Claude Code

@barjin
barjin requested a review from Pijukatel September 3, 2026 07:26
Comment thread .github/workflows/node-test.yaml Outdated
Comment thread impit-node/index.wrapper.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use textEncoder created on line 48?

Comment thread impit-node/index.wrapper.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use textEncoder created on line 48?

Comment thread impit-node/index.wrapper.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use textEncoder created on line 48?

@barjin
barjin merged commit 4d22b81 into master Sep 7, 2026
52 checks passed
@barjin
barjin deleted the perf/hot-path-allocations branch September 7, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants