-
Notifications
You must be signed in to change notification settings - Fork 111
[runtime/utils/buffer] refactor write.rs #1159
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
Conversation
9de215a
to
7ae44c4
Compare
runtime/src/utils/buffer/write.rs
Outdated
// Flush buffer if adding this data would exceed capacity. | ||
if buffer.data.len() + buf_len > buffer.capacity { | ||
let (buf, offset) = buffer.take_buf(); | ||
self.blob.write_at(buf, offset).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit here is that if the buffer is cleared and its offset advanced by take_buf() even if this write fails. Before the buffer would be cleared but its offset would not change, which would also be problematic if we tried to continue I think. In any case you probably have bigger problems if you're getting write errors, so probably not worth trying to make them recoverable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as before. I suppose we could handle it by just handing it back to the buffer 🤔
7ae44c4
to
cbd41c4
Compare
999bfb2
to
bf1756a
Compare
9213a6d
to
909a7f3
Compare
a6c6cc5
to
706ca88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a little more polish of the Buffer
API, I think this becomes meaningfully clearer than what is in main
(but at the moment think there is still too much intertwining).
939908d
to
4f2cdda
Compare
4f2cdda
to
f59ed9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the buffered write implementation by extracting buffer logic into a new Buffer
struct and moving blob read/write logic into the outer Write
type.
- Introduce
Buffer
with methods for sizing, resizing, taking, extracting, and merging buffered data. - Update
Write
to hold both the underlying blob and the new buffer, and adjust allBlob
trait implementations accordingly. - Improve naming consistency (e.g., use
offset
suffix) and address formatting nits.
Comments suppressed due to low confidence (2)
runtime/src/utils/buffer/write.rs:51
- The doc comment for
resize
describes behavior but does not document the returnedOption<(Vec<u8>, u64)>
. Please clarify what theVec<u8>
andu64
represent and whenNone
is returned to improve understandability.
fn resize(&mut self, len: u64) -> Option<(Vec<u8>, u64)> {
runtime/src/utils/buffer/write.rs:128
- The
merge
(and similarlyextract
) method contains complex logic with zero‐fill and offset calculations but lacks unit tests. Consider adding targeted tests for edge cases such as full overlap, no overlap, and gap zero‐fill scenarios.
fn merge(&mut self, data: &[u8], offset: u64) -> bool {
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1159 +/- ##
==========================================
- Coverage 91.11% 91.07% -0.05%
==========================================
Files 216 216
Lines 57831 57772 -59
==========================================
- Hits 52694 52617 -77
- Misses 5137 5155 +18
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Buffer
, and moving all logic pertaining to reading/writing the blob to the outer level. Logic pertaining to reading/writing from the buffer is moved within the Buffer's methods for a clear separation of concerns.