Re: [PR] perf(core): read log files asynchronously in batches, deleting the sync bridge [hudi-rs]
yihua commented on PR #686: URL: https://github.com/apache/hudi-rs/pull/686#issuecomment-5504924840 Closing this PR as the set of changes are reviewed in #689. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] perf(core): read log files asynchronously in batches, deleting the sync bridge [hudi-rs]
yihua closed pull request #686: perf(core): read log files asynchronously in batches, deleting the sync bridge URL: https://github.com/apache/hudi-rs/pull/686 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] perf(core): read log files asynchronously in batches, deleting the sync bridge [hudi-rs]
yihua commented on code in PR #686:
URL: https://github.com/apache/hudi-rs/pull/686#discussion_r3910873614
##
crates/core/src/storage/reader.rs:
##
@@ -240,62 +223,113 @@ impl StorageReader {
LogBlockFetcher::new(self.object_store.clone(), self.location.clone())
}
-/// Refill the window if the cursor has moved outside it. No-op past the
end.
-fn ensure_window(&mut self) -> Result<()> {
-if self.pos >= self.file_len {
-return Ok(());
-}
-let in_window = self.pos >= self.window_start
-&& self.pos < self.window_start + self.window.len() as u64;
-if in_window {
-return Ok(());
-}
-let start = self.pos;
-let end = (start + self.window_size).min(self.file_len);
-self.window = get_range_blocking(&self.object_store, &self.location,
start, end - start)?;
-self.window_start = start;
+/// Length of the whole file, known without reading any of it.
+pub fn file_len(&self) -> u64 {
+self.file_len
+}
+
+/// Where the cursor sits.
+pub fn position(&self) -> u64 {
+self.pos
+}
+
+/// Move the cursor. No I/O, and a position past the end of the file is
+/// allowed: it fails at the next read, which is what lets a caller skip
past
+/// a block's content without paying for it.
+pub fn seek_to(&mut self, pos: u64) {
+self.pos = pos;
+}
+
+/// Whether `[start, end)` lies inside the resident window.
+fn window_covers(&self, start: u64, end: u64) -> bool {
+start >= self.window_start && end <= self.window_start +
self.window.len() as u64
+}
+
+/// Fetch a window starting at the cursor.
+async fn fill_window(&mut self) -> Result<()> {
Review Comment:
non-blocking: Two fetch patterns here look worth a measurement before the
merge-down: on a file whose blocks are smaller than the window, the walk's
refills sweep essentially the whole file and Pass 3 then re-fetches the
admitted content, so total bytes read are ~2x the old whole-file GET; and for a
block larger than the window, `is_block_corrupted`'s trailing-pointer probe
evicts and re-fetches the header-side window (seek to trailing → refill → seek
back → refill again), costing ~2 extra windows per large block. Have you
considered a bytes-fetched counter in the object-storage bench you already
plan, or restoring/keeping the walk window across the corruption probe? The
memory win is measured and real — this is only about knowing what it costs in
bandwidth where round trips were the justification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] perf(core): read log files asynchronously in batches, deleting the sync bridge [hudi-rs]
yihua commented on PR #686: URL: https://github.com/apache/hudi-rs/pull/686#issuecomment-5504663108 non-blocking: one question on object-storage performance before this stack's numbers get quoted. The memory win is measured and convincing, but every performance-shaped claim is local so far: the PR's own account says wall time is 7-10% slower on a local filesystem (attributed to lost OS readahead), the round-trip saving from `get_ranges` coalescing is unmeasured, and the header walk's windowed refills change the byte and round-trip profile in ways a local disk cannot show at all. Is an S3 (or other object-store) run of the fg-bench/memory-bench pair planned before the merge-down — ideally with a bytes-fetched counter, which would also answer the window-refill question on `fill_window` above in the same run? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] perf(core): read log files asynchronously in batches, deleting the sync bridge [hudi-rs]
yihua commented on code in PR #686:
URL: https://github.com/apache/hudi-rs/pull/686#discussion_r3910873614
##
crates/core/src/storage/reader.rs:
##
@@ -240,62 +223,113 @@ impl StorageReader {
LogBlockFetcher::new(self.object_store.clone(), self.location.clone())
}
-/// Refill the window if the cursor has moved outside it. No-op past the
end.
-fn ensure_window(&mut self) -> Result<()> {
-if self.pos >= self.file_len {
-return Ok(());
-}
-let in_window = self.pos >= self.window_start
-&& self.pos < self.window_start + self.window.len() as u64;
-if in_window {
-return Ok(());
-}
-let start = self.pos;
-let end = (start + self.window_size).min(self.file_len);
-self.window = get_range_blocking(&self.object_store, &self.location,
start, end - start)?;
-self.window_start = start;
+/// Length of the whole file, known without reading any of it.
+pub fn file_len(&self) -> u64 {
+self.file_len
+}
+
+/// Where the cursor sits.
+pub fn position(&self) -> u64 {
+self.pos
+}
+
+/// Move the cursor. No I/O, and a position past the end of the file is
+/// allowed: it fails at the next read, which is what lets a caller skip
past
+/// a block's content without paying for it.
+pub fn seek_to(&mut self, pos: u64) {
+self.pos = pos;
+}
+
+/// Whether `[start, end)` lies inside the resident window.
+fn window_covers(&self, start: u64, end: u64) -> bool {
+start >= self.window_start && end <= self.window_start +
self.window.len() as u64
+}
+
+/// Fetch a window starting at the cursor.
+async fn fill_window(&mut self) -> Result<()> {
Review Comment:
non-blocking: Two fetch patterns here look worth a measurement before the
merge-down: on a file whose blocks are smaller than the window, the walk's
refills sweep essentially the whole file and Pass 3 then re-fetches the
admitted content, so total bytes read are ~2x the old whole-file GET; and for a
block larger than the window, `is_block_corrupted`'s trailing-pointer probe
evicts and re-fetches the header-side window (seek to trailing → refill → seek
back → refill again), costing ~2 extra windows per large block. Have you
considered a bytes-fetched counter in the object-storage bench you already
plan, or restoring/keeping the walk window across the corruption probe? The
memory win is measured and real — this is only about knowing what it costs in
bandwidth where round trips were the justification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
