Skip to content

Conversation

@clabby
Copy link
Collaborator

@clabby clabby commented Dec 15, 2025

Overview

Adds a BufferedSender wrapper in stream that reduces memory fragmentation by re-using a buffer that grows to the size of the largest message sent.

send_frame (unbuffered)
Screenshot 2025-12-15 at 2 54 09 AM

BufferedSender
Screenshot 2025-12-15 at 2 53 41 AM

@clabby clabby self-assigned this Dec 15, 2025
@clabby clabby added this to Tracker Dec 15, 2025
@clabby clabby moved this to Ready for Review in Tracker Dec 15, 2025
if self.send_buf.capacity() < target_capacity {
self.send_buf
.reserve(target_capacity - self.send_buf.capacity());
}
Copy link

Choose a reason for hiding this comment

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

Bug: Reserve calculation incorrectly computes additional capacity needed

The reserve() call uses target_capacity - self.send_buf.capacity() but BytesMut::reserve(n) ensures capacity >= len + n. After clear(), len is 0, so reserve(n) guarantees capacity >= n. When capacity is between half the target and the target (e.g., capacity=150, target=200), calling reserve(50) only guarantees capacity ≥ 50, which is already satisfied, so nothing happens. The intended 2x buffer optimization fails to work, causing the same memory fragmentation the BufferedSender was designed to prevent.

Fix in Cursor Fix in Web

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is incorrect; BytesMut::reserve documents:

Reserves capacity for at least additional more bytes to be inserted into the given BytesMut.

More than additional bytes may be reserved in order to avoid frequent reallocations. A call to reserve may result in an allocation.

@clabby
Copy link
Collaborator Author

clabby commented Dec 15, 2025

#2518 potential alternative

@clabby
Copy link
Collaborator Author

clabby commented Dec 15, 2025

It's worth approaching vectored writes instead of this IMO. Yields a performance benefit for sure, but is more of a band-aid when a larger API change is justified.

@clabby clabby closed this Dec 15, 2025
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Tracker Dec 15, 2025
@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

❌ Patch coverage is 89.77273% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.50%. Comparing base (2be92d6) to head (008de79).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
stream/src/utils/codec.rs 89.41% 9 Missing ⚠️
@@            Coverage Diff             @@
##             main    #2512      +/-   ##
==========================================
- Coverage   92.51%   92.50%   -0.02%     
==========================================
  Files         340      340              
  Lines       97407    97485      +78     
==========================================
+ Hits        90114    90174      +60     
- Misses       7293     7311      +18     
Files with missing lines Coverage Δ
stream/src/lib.rs 95.67% <100.00%> (-0.16%) ⬇️
stream/src/utils/codec.rs 95.69% <89.41%> (-4.31%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2be92d6...008de79. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants