Refactor FilePrefetchBuffer code #12097

akankshamahajan15 · 2023-11-27T01:20:00Z

Summary - Refactor FilePrefetchBuffer code

Implementation:
FilePrefetchBuffer maintains a deque of free buffers (free_bufs_) of size num_buffers_ and buffers (bufs_) which contains the prefetched data. Whenever a buffer is consumed or is outdated (w.r.t. to requested offset), that buffer is cleared and returned to free_bufs_.

If a buffer is available in free_bufs_, it's moved to bufs_ and is sent for prefetching. num_buffers_ defines how many buffers are maintained that contains prefetched data.
If num_buffers_ == 1, it's a sequential read flow. Read API will be called on that one buffer whenever the data is requested and is not in the buffer.
If num_buffers_ > 1, then the data is prefetched asynchronosuly in the buffers whenever the data is consumed from the buffers and that buffer is freed.
If num_buffers > 1, then requested data can be overlapping between 2 buffers. To return the continuous buffer overlap_bufs_ is used. The requested data is copied from 2 buffers to the overlap_bufs_ and overlap_bufs_ is returned to
the caller.

Merged Sync and Async code flow into one in FilePrefetchBuffer.

Test Plan -

Crash test passed
Unit tests
Pending - Benchmarks

facebook-github-bot · 2023-12-01T17:43:11Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-12-01T18:21:11Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-01T19:52:25Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-01T19:52:48Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-01T20:05:49Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-01T20:39:36Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-04T17:45:41Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-04T18:26:31Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-04T18:39:28Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-04T19:07:42Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

file/file_prefetch_buffer.h

facebook-github-bot · 2023-12-04T23:32:08Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-04T23:36:05Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-05T00:05:56Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-15T01:00:19Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-15T01:05:34Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-15T04:22:23Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-12-15T04:43:06Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

ajkr

Please feel free to leave [existing issue] comments for another time; I just recorded them here since reviewing this PR made them visible.

file/file_prefetch_buffer.cc

akankshamahajan15 · 2023-12-18T17:56:32Z

NOTE : Some of the stress tests are failing for async_io after refactoring. Once that's fixed (most likely tomorrow), I'll address the comments and update the diff on Wednesday.

ajkr

Finished a complete pass.

file/file_prefetch_buffer.cc

ajkr · 2023-12-18T19:00:21Z

file/file_prefetch_buffer.cc

  // Check if the first buffer has the required offset and the async read is
  // still in progress. This should only happen if a prefetch was initiated
  // by Seek, but the next access is at another offset.


Where is poll supposed to happen if the access is at the same offset? It seems to happen here whenever there's two buffers.

Yes. So the data is overlapping between 2 buffers. Offset lies in first buffer so first buffer has the data but async is in progress for that buffer. So we call Poll here to get the data first and then copy the data to overlap_buf_.

We assumption was Poll is supposed to happen in PrefetchInternal.
But for seek operation, we call PrefetchAsync to submit async requests for all L0 files and L1-Ln levels. And then it calls again (second call) FilePrefetchBuffer to actually poll and read the data. It can be possible that second call wasn't made as data block was found in cache. So the buffer is never polled.

This case handle that condition.
In short - If buffer is needed but async is in progress, poll the data.

It can be possible that second call wasn't made as data block was found in cache.

The second call when there's a cache miss is like this, right? SeekSecondPass() -> ... -> TryReadFromCache() -> ... -> PrefetchInternal()

I am seeing the code here poll even in that case, which is fine but seems to contradict the "next access is at another offset" part of the comment

facebook-github-bot · 2023-12-19T22:54:27Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

ajkr

LGTM! I don't have any concerns but hope we will continue simplifying it

ajkr · 2023-12-21T05:24:21Z

file/file_prefetch_buffer.cc

+  // In case buffers do not align, reset next buffer if requested data needs
+  // to be read in that buffer.
+  if (NumBuffersAllocated() > 1) {


This logic feels more like it belongs to HandleOverlappingData(). For the caller that does not also call HandleOverlappingData() (PrefetchAsync()), it looks like this might not be needed?

ajkr · 2023-12-21T06:07:36Z

file/file_prefetch_buffer.cc

+    // NOTE - After this poll request, first buffer might be empty because of
+    // IOError in callback while reading or it may contains the required data.
+    PollIfNeeded(offset, length);


The above comment says "Call Poll only if data is needed for the second buffer". But here we call it the first buffer.

I sort of suspect the original comment referred to the "second buffer" as the second buffer from which bytes in [offset, offset + length) are read. At least according to my understanding, at this point, the front buffer would only need to be polled if HandleOverlappingData() had already freed the buffer it polled (the "first buffer" in my understanding).

ajkr · 2023-12-21T07:02:46Z

file/file_prefetch_buffer.cc

+    // NOTE - After this poll request, first buffer might be empty because of
+    // IOError in callback while reading or it may contains the required data.
+    PollIfNeeded(offset, length);
  }

-  if (copy_to_third_buffer) {
+  if (copy_to_overlap_buffer) {
    offset = tmp_offset;
    length = tmp_length;
  }

-  // 4. After polling and swapping buffers, if all the requested bytes are in
-  // curr_, it will only go for async prefetching.
-  // copy_to_third_buffer is a special case so it will be handled separately.
-  if (!copy_to_third_buffer && DoesBufferContainData(curr_) &&
-      IsDataBlockInBuffer(offset, length, curr_)) {
-    offset += length;
-    length = 0;
-
-    // Since async request was submitted directly by calling PrefetchAsync in
-    // last call, we don't need to prefetch further as this call is to poll
-    // the data submitted in previous call.
-    if (explicit_prefetch_submitted_) {
-      return s;
-    }
-    if (!IsSecondBuffEligibleForPrefetching()) {
-      UpdateStats(/*found_in_buffer=*/true, original_length);
-      return s;
+  // After polling, if all the requested bytes are in first buffer, it will only
+  // go for async prefetching.
+  if (buf->DoesBufferContainData()) {
+    if (copy_to_overlap_buffer) {
+      // Data is overlapping i.e. some of the data has been copied to overlap
+      // buffer and remaining will be updated below.
+      size_t initial_buf_size = overlap_buf_->CurrentSize();
+      CopyDataToBuffer(buf, offset, length);
+      UpdateStats(
+          /*found_in_buffer=*/false,
+          overlap_buf_->CurrentSize() - initial_buf_size);
+
+      // Length == 0: All the requested data has been copied to overlap buffer
+      // and it has already gone for async prefetching. It can return without
+      // doing anything further. Length > 0: More data needs to be consumed so
+      // it will continue async and sync prefetching and copy the remaining data
+      // to overlap buffer in the end.
+      if (length == 0) {
+        UpdateStats(/*found_in_buffer=*/true, length);
+        return s;
+      }


I wonder if this belongs in HandleOverlappingData(). It's hard to say where that function should end and this one picks up. If we say logic specific to overlapping buffers belongs there, then we should probably move this.

facebook-github-bot · 2023-12-26T18:59:11Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-01-05T01:44:21Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-01-05T01:45:07Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2024-01-05T05:56:32Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2024-01-05T06:00:17Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-01-05T06:30:29Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-01-05T17:33:09Z

@akankshamahajan15 merged this pull request in 5cb2d09.

facebook-github-bot added the CLA Signed label Nov 27, 2023

akankshamahajan15 requested a review from anand1976 November 27, 2023 03:50

akankshamahajan15 force-pushed the refactor_FBP branch 3 times, most recently from 41f41b1 to f73c6de Compare December 1, 2023 00:19

akankshamahajan15 force-pushed the refactor_FBP branch from ae06594 to dc62c1a Compare December 1, 2023 18:21

akankshamahajan15 force-pushed the refactor_FBP branch from dc62c1a to cee3f5c Compare December 1, 2023 19:52

akankshamahajan15 force-pushed the refactor_FBP branch from cee3f5c to 5040552 Compare December 1, 2023 19:52

akankshamahajan15 force-pushed the refactor_FBP branch from 5040552 to 8cef5f0 Compare December 1, 2023 20:05

akankshamahajan15 force-pushed the refactor_FBP branch from 8cef5f0 to 6d786ac Compare December 1, 2023 20:39

akankshamahajan15 changed the title ~~[RFC] [WIP] Refactor FilePrefetchBuffer code~~ Refactor FilePrefetchBuffer code Dec 4, 2023

akankshamahajan15 force-pushed the refactor_FBP branch from 6d786ac to 873507a Compare December 4, 2023 17:45

akankshamahajan15 force-pushed the refactor_FBP branch from 873507a to 07b9367 Compare December 4, 2023 18:26

akankshamahajan15 commented Dec 4, 2023

View reviewed changes

file/file_prefetch_buffer.h Outdated Show resolved Hide resolved

akankshamahajan15 commented Dec 4, 2023

View reviewed changes

file/file_prefetch_buffer.h Outdated Show resolved Hide resolved

akankshamahajan15 force-pushed the refactor_FBP branch from d76e301 to a44a8c4 Compare December 4, 2023 23:36

akankshamahajan15 force-pushed the refactor_FBP branch from a44a8c4 to 29666fe Compare December 5, 2023 00:05

akankshamahajan15 force-pushed the refactor_FBP branch from 79174c1 to e8c6b2e Compare December 15, 2023 04:22

akankshamahajan15 force-pushed the refactor_FBP branch from e8c6b2e to 3332598 Compare December 15, 2023 04:43

ajkr reviewed Dec 15, 2023

View reviewed changes

file/file_prefetch_buffer.cc Outdated Show resolved Hide resolved

file/file_prefetch_buffer.cc Outdated Show resolved Hide resolved

file/file_prefetch_buffer.cc Show resolved Hide resolved

file/file_prefetch_buffer.cc Outdated Show resolved Hide resolved

ajkr reviewed Dec 18, 2023

View reviewed changes

akankshamahajan15 force-pushed the refactor_FBP branch from 3332598 to 8eee7ec Compare December 19, 2023 22:54

ajkr approved these changes Dec 21, 2023

View reviewed changes

akankshamahajan15 force-pushed the refactor_FBP branch from 8eee7ec to 419fbd5 Compare January 5, 2024 01:44

akankshamahajan15 added 4 commits January 4, 2024 21:52

FilePrefetchBuffer refactoring

001443c

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Addressed comments

20c2e83

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Format code

b864d1d

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Addressed additional comments

f037302

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the refactor_FBP branch from 419fbd5 to 01aa328 Compare January 5, 2024 05:56

Fix stress test failures and addressed comments

c4891d8

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the refactor_FBP branch from 01aa328 to c4891d8 Compare January 5, 2024 06:00

facebook-github-bot closed this in 5cb2d09 Jan 5, 2024

facebook-github-bot added the Merged label Jan 5, 2024

Refactor FilePrefetchBuffer code #12097

Refactor FilePrefetchBuffer code #12097

Conversation

akankshamahajan15 commented Nov 27, 2023 • edited Loading

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 4, 2023

facebook-github-bot commented Dec 4, 2023

facebook-github-bot commented Dec 4, 2023

facebook-github-bot commented Dec 4, 2023

facebook-github-bot commented Dec 4, 2023

facebook-github-bot commented Dec 4, 2023

facebook-github-bot commented Dec 5, 2023

facebook-github-bot commented Dec 15, 2023

facebook-github-bot commented Dec 15, 2023

facebook-github-bot commented Dec 15, 2023

facebook-github-bot commented Dec 15, 2023

ajkr left a comment

Choose a reason for hiding this comment

akankshamahajan15 commented Dec 18, 2023

ajkr left a comment

Choose a reason for hiding this comment

ajkr Dec 18, 2023

Choose a reason for hiding this comment

akankshamahajan15 Dec 21, 2023

Choose a reason for hiding this comment

ajkr Dec 21, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Dec 19, 2023

ajkr left a comment

Choose a reason for hiding this comment

ajkr Dec 21, 2023

Choose a reason for hiding this comment

ajkr Dec 21, 2023

Choose a reason for hiding this comment

ajkr Dec 21, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Dec 26, 2023

facebook-github-bot commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

akankshamahajan15 commented Nov 27, 2023 •

edited

Loading