Pre-allocate the buffer based on the expected Content-Length with the Rust HTTP client#19498
Conversation
…he Rust HTTP client
rust/src/http_client.rs
Outdated
| let content_length = response | ||
| .headers() | ||
| .get(reqwest::header::CONTENT_LENGTH) |
There was a problem hiding this comment.
For reference, response.content_length() does exist but
This value does not directly represents the value of the
Content-Lengthheader, but rather the size of the response’s body. To read the header’s value, please use theResponse::headersmethod instead.
We want to avoid reading the entire body at this point as we stream it below to avoid allocating a giant object on the server as we want to reject anything above the response_limit.
There was a problem hiding this comment.
I am a bit sceptical of that manual chunk aggregation though.
Fine with making it better in another PR, but we could:
- extract the underlying body with
axum::Body::from(response) - wrap it with
http_body_util::Limited::new(body, limit) - collect it in a list of buffers like
limited_body.collect().await - convert it to a
bytes::Bytes - return that directly, making sure we have the right feature enabled for
pyo3to support its conversion
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
There was a problem hiding this comment.
Previous discussion on using http_body_util::Limited #18357 (comment)
There was a problem hiding this comment.
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::BytestoPyBytesis also done without copy.
As the docs actually say this:
When converting
Bytesback into Python, this will do a copy, just like&[u8]andVec<u8>.
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)
There was a problem hiding this comment.
Let's still go for merging this PR even though #19510 supersedes it after
|
Thanks for the review and plan for a better future @sandhose 🐸 |
*As suggested by @sandhose in #19498 (comment) Simplify Rust HTTP client response streaming and limiting ### Dev notes Synapse's Rust HTTP client was introduced in #18357 ### Pull Request Checklist <!-- Please read https://element-hq.github.io/synapse/latest/development/contributing_guide.html before submitting your pull request --> * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [element-hq/synapse](https://github.com/element-hq/synapse) | minor | `v1.148.0` → `v1.149.0` | --- ### Release Notes <details> <summary>element-hq/synapse (element-hq/synapse)</summary> ### [`v1.149.0`](https://github.com/element-hq/synapse/releases/tag/v1.149.0) [Compare Source](element-hq/synapse@v1.148.0...v1.149.0) ### Synapse 1.149.0 (2026-03-10) No significant changes since 1.149.0rc1. ### Synapse 1.149.0rc1 (2026-03-03) #### Features - Add experimental support for [MSC4388: Secure out-of-band channel for sign in with QR](matrix-org/matrix-spec-proposals#4388). ([#​19127](element-hq/synapse#19127)) - Add stable support for [MSC4380](matrix-org/matrix-spec-proposals#4380) invite blocking. ([#​19431](element-hq/synapse#19431)) #### Bugfixes - Fix the 'Login as a user' Admin API not checking if the user exists before issuing an access token. ([#​18518](element-hq/synapse#18518)) - Fix `/sync` missing membership event in `state_after` (experimental [MSC4222](matrix-org/matrix-spec-proposals#4222) implementation) in some scenarios. ([#​19460](element-hq/synapse#19460)) #### Internal Changes - Add log to explain when and why we freeze objects in the garbage collector. ([#​19440](element-hq/synapse#19440)) - Better instrument `JoinRoomAliasServlet` with tracing. ([#​19461](element-hq/synapse#19461)) - Fix Complement CI not running against the code from our PRs. ([#​19475](element-hq/synapse#19475)) - Log `docker system info` in CI so we have a plain record of how GitHub runners evolve over time. ([#​19480](element-hq/synapse#19480)) - Rename the `test_disconnect` test helper so that pytest doesn't see it as a test. ([#​19486](element-hq/synapse#19486)) - Add a log line when we delete devices. Contributed by [@​bradtgmurray](https://github.com/bradtgmurray) @​ Beeper. ([#​19496](element-hq/synapse#19496)) - Pre-allocate the buffer based on the expected `Content-Length` with the Rust HTTP client. ([#​19498](element-hq/synapse#19498)) - Cancel long-running sync requests if the client has gone away. ([#​19499](element-hq/synapse#19499)) - Try and reduce reactor tick times when under heavy load. ([#​19507](element-hq/synapse#19507)) - Simplify Rust HTTP client response streaming and limiting. ([#​19510](element-hq/synapse#19510)) - Replace deprecated collection import locations with current locations. ([#​19515](element-hq/synapse#19515)) - Bump most locked Python dependencies to their latest versions. ([#​19519](element-hq/synapse#19519)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41OS4yIiwidXBkYXRlZEluVmVyIjoiNDMuNTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiaW1hZ2UiXX0=--> Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/4580 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>
Pre-allocate the buffer based on the expected
Content-Lengthwith the Rust HTTP clientSpawning from looking at some traces and seeing the Synapse Rust HTTP client taking way longer than what the Synapse Pro Event Cache claims it was able to respond in (added some better tracing for that). I don't think this specific change will have a meaningful impact but just something I saw (pre-optimization).
Example Trace-845fb3-2026-02-27 16_32_24.json
Dev notes
This kind of delay also reminds me of
TCP_NODELAYproblems but we already define that on the Synapse Pro Event Cache side.u64/usizePull Request Checklist
EventStoretoEventWorkerStore.".code blocks.