Account already downloaded data when resetting a prefetcher #797

vladem · 2024-03-04T20:27:12Z

Description of change

This is a development if an idea from this PR: #795

The idea is to avoid discarding prefetched data if it is already downloaded. This optimises a certain read pattern, which reads data sequentially but skips chunks between reads.

Here is some benchmarking with fio job included in the pr (no cache usage, remounted in between fio invocations):

# MP built from main
$ fio --directory=/home/vlaad/mounted_s3 /home/vlaad/local/mountpoint-s3/mountpoint-s3/scripts/fio/read/seq_read_skip_read.fio | grep "bw="
  read : io=5888.0MB, bw=20035KB/s, iops=0, runt=300944msec
$ !!
  read : io=5888.0MB, bw=19642KB/s, iops=0, runt=306958msec
$ !!
  read : io=6400.0MB, bw=20691KB/s, iops=0, runt=316740msec

# MP built from this branch 
$ fio --directory=/home/vlaad/mounted_s3 /home/vlaad/local/mountpoint-s3/mountpoint-s3/scripts/fio/read/seq_read_skip_read.fio
  read : io=47616MB, bw=160757KB/s, iops=0, runt=303308msec
$ !!
  read : io=41728MB, bw=142388KB/s, iops=0, runt=300092msec
$ !!
  read : io=51712MB, bw=172735KB/s, iops=0, runt=306557msec

upd. updated benchmark results with longer runtime 300s and larger file 10gb, lowest observed improvement is 6.8x

Relevant issues: No

Does this change impact existing behavior?

This is not a functional change and thus not a breaking one. We should reason about possible performance regressions in certain read patterns before merging.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

Signed-off-by: Vladislav Volodkin <[email protected]>

jamesbornholt

I like this idea better than #795. Just a couple notes.

mountpoint-s3/src/prefetch.rs

mountpoint-s3/src/prefetch/part_queue.rs

Signed-off-by: Vladislav Volodkin <[email protected]>

passaro · 2024-03-05T15:59:02Z

mountpoint-s3/src/prefetch/part_queue.rs

    let part_queue = PartQueue {
        current_part: AsyncMutex::new(None),
        receiver,
        failed: AtomicBool::new(false),
-        downloaded: Arc::clone(&downloaded),
+        bytes_arrived_total: Arc::clone(&bytes_counter),


nit: maybe bytes_received?

well, I put some thinking in that tbh! bytes_received may be misleading because of PartQueue.receiver, which may imply that those bytes were received (i.e read through receiver), but what it actually tracks is the number of bytes arrived to the queue (which may also not have the clearest meaning, naming is complicated)

Well, they are the bytes received by the receiver, so it still sounds good to be. I'd rather clarify in the rustdoc than add a different name.

I liked bytes_downloaded, but also not worth bikeshedding over the name.

I'll replace with bytes_received, may be not completely accurate, but symmetrical to the sending part and there is a comment with a clarification

passaro · 2024-03-05T16:25:48Z

mountpoint-s3/src/prefetch.rs

+        // it. Otherwise, we're willing to wait for the bytes to download only if they're coming "soon", where
+        // soon is defined as up to `max_forward_seek_distance` bytes ahead of the available offset.
+        let available_offset = current_task.available_offset();
+        if offset > available_offset.saturating_add(self.config.max_forward_seek_distance) {


max_forward_seek_distance is now the maximum number of bytes we are happy to wait to be downloaded when seeking forward. Can we think of a more appropriate name?

max_forward_seek_wait_distance?

Also, there's a subtle thing here that it doesn't matter if available_offset + max_forward_seek_wait_distance > current_task.end_offset(), because the loop above guarantees that the offset lands within this task in the loop above. Maybe worth a comment on the line above this one that selects current_task?

oh, yeah, not regarding this particular line, but I also thought about the case when offset is beyond the last task's range, I'll add a comment and will try to add a test (if possible without too much hacks)

max_forward_seek_wait_distance looks good to me, can not think about anything less wordy

jamesbornholt

Yeah I like this. We should think about how we want metrics for the prefetcher to work now, but let's do that in a separate PR.

jamesbornholt · 2024-03-05T22:38:25Z

mountpoint-s3/src/prefetch.rs

+        // it. Otherwise, we're willing to wait for the bytes to download only if they're coming "soon", where
+        // soon is defined as up to `max_forward_seek_distance` bytes ahead of the available offset.
+        let available_offset = current_task.available_offset();
+        if offset > available_offset.saturating_add(self.config.max_forward_seek_distance) {


max_forward_seek_wait_distance?

Also, there's a subtle thing here that it doesn't matter if available_offset + max_forward_seek_wait_distance > current_task.end_offset(), because the loop above guarantees that the offset lands within this task in the loop above. Maybe worth a comment on the line above this one that selects current_task?

mountpoint-s3/src/prefetch/part_queue.rs

jamesbornholt · 2024-03-05T22:45:51Z

mountpoint-s3/src/prefetch/part_queue.rs

    let part_queue = PartQueue {
        current_part: AsyncMutex::new(None),
        receiver,
        failed: AtomicBool::new(false),
-        downloaded: Arc::clone(&downloaded),
+        bytes_arrived_total: Arc::clone(&bytes_counter),


I liked bytes_downloaded, but also not worth bikeshedding over the name.

mountpoint-s3/src/prefetch/part_queue.rs

Signed-off-by: Vladislav Volodkin <[email protected]>

passaro · 2024-03-06T20:05:54Z

mountpoint-s3/src/prefetch/part_queue.rs

-        let send_result = self.sender.send_blocking(part);
-        if let Some(part_size) = part_size {
-            self.bytes_sent.fetch_add(part_size, Ordering::SeqCst);
+        if let Ok(ref part) = part {


nit: you can also use:

Suggested change

if let Ok(ref part) = part {

if let Ok(part) = &part {

Account already downloaded data when resetting a prefetcher

be2e1ca

Signed-off-by: Vladislav Volodkin <[email protected]>

vladem temporarily deployed to PR integration tests March 4, 2024 20:27 — with GitHub Actions Inactive

vladem had a problem deploying to PR integration tests March 4, 2024 20:27 — with GitHub Actions Failure

vladem temporarily deployed to PR integration tests March 4, 2024 20:27 — with GitHub Actions Inactive

jamesbornholt reviewed Mar 4, 2024

View reviewed changes

mountpoint-s3/src/prefetch.rs Outdated Show resolved Hide resolved

mountpoint-s3/src/prefetch/part_queue.rs Outdated Show resolved Hide resolved

passaro mentioned this pull request Mar 5, 2024

Proof-of-concept prefetcher optimization for some non-sequential read patterns #795

Closed

vladem had a problem deploying to PR integration tests March 5, 2024 15:47 — with GitHub Actions Failure

Refactor to use absolute offsets

451b2a2

Signed-off-by: Vladislav Volodkin <[email protected]>

vladem force-pushed the optimize-read-skip-read branch from 4a302da to 451b2a2 Compare March 5, 2024 15:51

vladem temporarily deployed to PR integration tests March 5, 2024 15:51 — with GitHub Actions Inactive

vladem mentioned this pull request Mar 5, 2024

Add fio job to benchmark read-skip-read pattern #799

Merged

passaro reviewed Mar 5, 2024

View reviewed changes

jamesbornholt reviewed Mar 5, 2024

View reviewed changes

vladem temporarily deployed to PR integration tests March 6, 2024 18:21 — with GitHub Actions Inactive

vladem marked this pull request as ready for review March 6, 2024 18:22

Be precise when checking available data

c3be223

Signed-off-by: Vladislav Volodkin <[email protected]>

vladem temporarily deployed to PR integration tests March 6, 2024 18:38 — with GitHub Actions Inactive

vladem had a problem deploying to PR integration tests March 6, 2024 18:38 — with GitHub Actions Failure

vladem temporarily deployed to PR integration tests March 6, 2024 18:38 — with GitHub Actions Inactive

vladem requested a review from passaro March 6, 2024 18:39

vladem temporarily deployed to PR integration tests March 6, 2024 20:03 — with GitHub Actions Inactive

passaro reviewed Mar 6, 2024

View reviewed changes

passaro approved these changes Mar 6, 2024

View reviewed changes

jamesbornholt approved these changes Mar 7, 2024

View reviewed changes

jamesbornholt added this pull request to the merge queue Mar 7, 2024

Merged via the queue into awslabs:main with commit 896c6d3 Mar 7, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account already downloaded data when resetting a prefetcher #797

Account already downloaded data when resetting a prefetcher #797

vladem commented Mar 4, 2024 •

edited

Loading

jamesbornholt left a comment

passaro Mar 5, 2024

vladem Mar 5, 2024 •

edited

Loading

passaro Mar 5, 2024

jamesbornholt Mar 5, 2024

vladem Mar 6, 2024

passaro Mar 5, 2024

jamesbornholt Mar 5, 2024

vladem Mar 6, 2024

jamesbornholt left a comment

jamesbornholt Mar 5, 2024

jamesbornholt Mar 5, 2024

passaro Mar 6, 2024

Account already downloaded data when resetting a prefetcher #797

Account already downloaded data when resetting a prefetcher #797

Conversation

vladem commented Mar 4, 2024 • edited Loading

Description of change

Does this change impact existing behavior?

jamesbornholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladem Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladem commented Mar 4, 2024 •

edited

Loading

vladem Mar 5, 2024 •

edited

Loading