Skip to content

Conversation

roberto-bayardo
Copy link
Collaborator

@roberto-bayardo roberto-bayardo commented Jun 25, 2025

  • move Blob out of the Inner struct, renaming Inner to 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.
  • add take_blob/clone_blob methods to support moving the blob among different caching wrappers as caching needs change due to usage.
  • optimization: avoids flushing the buffer in the event a written range precedes the buffer entirely. there remains some inefficiency when there is partial overlap between the written range and the buffer, since the buffer is flushed only to just immediately write over some of that just-written data, but probably not worth adding complexity to eliminate this edge case.
  • improve variable naming consistency, eg. offsets into the blob now always have an "offset" suffix to disambiguate them from offsets into other structures such as the various buffers.
  • address formatting nits.

@roberto-bayardo roberto-bayardo force-pushed the refactor-write-take-2 branch 19 times, most recently from 9de215a to 7ae44c4 Compare June 25, 2025 22:46
// 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?;
Copy link
Collaborator Author

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.

Copy link
Contributor

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 🤔

Copilot

This comment was marked as outdated.

@roberto-bayardo roberto-bayardo force-pushed the refactor-write-take-2 branch 7 times, most recently from 999bfb2 to bf1756a Compare June 26, 2025 17:39
@roberto-bayardo roberto-bayardo force-pushed the refactor-write-take-2 branch from 9213a6d to 909a7f3 Compare June 26, 2025 22:11
@roberto-bayardo roberto-bayardo force-pushed the refactor-write-take-2 branch 3 times, most recently from a6c6cc5 to 706ca88 Compare June 26, 2025 22:33
Copy link
Contributor

@patrick-ogrady patrick-ogrady left a 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).

@roberto-bayardo roberto-bayardo force-pushed the refactor-write-take-2 branch 3 times, most recently from 939908d to 4f2cdda Compare June 27, 2025 15:19
@roberto-bayardo roberto-bayardo force-pushed the refactor-write-take-2 branch from 4f2cdda to f59ed9a Compare June 27, 2025 16:08
@patrick-ogrady patrick-ogrady requested a review from Copilot June 29, 2025 22:15
Copy link
Contributor

@Copilot Copilot AI left a 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 all Blob 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 returned Option<(Vec<u8>, u64)>. Please clarify what the Vec<u8> and u64 represent and when None 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 similarly extract) 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 {

@patrick-ogrady patrick-ogrady merged commit 86a882c into main Jun 30, 2025
28 checks passed
@patrick-ogrady patrick-ogrady deleted the refactor-write-take-2 branch June 30, 2025 00:43
@codecov
Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 91.07143% with 10 lines in your changes missing coverage. Please review.

Project coverage is 91.07%. Comparing base (af8473f) to head (2ebc915).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
runtime/src/utils/buffer/write.rs 91.07% 10 Missing ⚠️
@@            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     
Files with missing lines Coverage Δ
runtime/src/utils/buffer/write.rs 93.71% <91.07%> (-5.69%) ⬇️

... and 5 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 fcdf0ab...2ebc915. 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

None yet

Development

Successfully merging this pull request may close these issues.

3 participants