[GitHub] [arrow] fsaintjacques commented on a change in pull request #7172: ARROW-8763: [C++] Add RandomAccessFile::WillNeed

[GitBox <gi...@apache.org>]

fsaintjacques commented on a change in pull request #7172:
URL: https://github.com/apache/arrow/pull/7172#discussion_r425106193



##########
File path: cpp/src/arrow/io/caching.cc
##########
@@ -171,7 +171,8 @@ Status ReadRangeCache::Cache(std::vector<ReadRange> ranges) {
   }
 
   impl_->AddEntries(std::move(entries));
-  return Status::OK();
+  // Prefetch immediately, regardless of executor availability, if possible
+  return impl_->file->WillNeed(ranges);

Review comment:
       I don't think it's necessary here since ReadRangeCache is effectively  a userland implementation of what the kernel does with fadvise. Second, the ordering here makes this call useless, i.e. the previous ReadAsync already hinted the kernel that you're reading said ranges.
   
   What I would like to see (in a following patch) is that ReadRangeCache is hidden via a custom CachedRandomAccessFile wrapper where`WillNeed` calls this dispatch. This would simplify the Parquet code path with buffering, e.g. we could remove all PreBuffering branch and just create a custom CachedRandomAccessFile once. We'd make the reader _always_ call `file->WillNeed(ranges)` regardless of the file system type.
   
   @lidavidm, tell me what do you think, and I'll open a ticket.

##########
File path: cpp/src/arrow/io/file.cc
##########
@@ -636,6 +665,9 @@ Result<std::shared_ptr<Buffer>> MemoryMappedFile::ReadAt(int64_t position,
 
   ARROW_ASSIGN_OR_RAISE(
       nbytes, internal::ValidateReadRange(position, nbytes, memory_map_->size()));
+  // Arrange to page data in
+  RETURN_NOT_OK(::arrow::internal::MemoryAdviseWillNeed(

Review comment:
       Callers that want this behaviour will call WillNeed pre-emptively irregardless of the RandomAccessFile sub-class. By doing it here, you're doing a syscall for every ReadAt defeating one of the purposes of using mmap.

##########
File path: cpp/src/arrow/io/memory.cc
##########
@@ -321,6 +342,11 @@ Result<std::shared_ptr<Buffer>> BufferReader::DoReadAt(int64_t position, int64_t
 
   ARROW_ASSIGN_OR_RAISE(nbytes, internal::ValidateReadRange(position, nbytes, size_));
   DCHECK_GE(nbytes, 0);
+

Review comment:
       Same comment regarding ReatAt with mmap file.

##########
File path: cpp/src/arrow/util/io_util.cc
##########
@@ -1072,6 +1072,61 @@ Status MemoryMapRemap(void* addr, size_t old_size, size_t new_size, int fildes,
 #endif
 }
 
+Status MemoryAdviseWillNeed(const std::vector<MemoryRegion>& regions) {
+  const auto page_size = static_cast<size_t>(GetPageSize());
+  DCHECK_GT(page_size, 0);
+  const size_t page_mask = ~(page_size - 1);
+  DCHECK_EQ(page_mask & page_size, page_size);
+
+  auto align_region = [=](const MemoryRegion& region) -> MemoryRegion {
+    const auto addr = reinterpret_cast<uintptr_t>(region.addr);
+    const auto aligned_addr = addr & page_mask;
+    DCHECK_LT(addr - aligned_addr, page_size);
+    return {reinterpret_cast<void*>(aligned_addr),
+            region.size + static_cast<size_t>(addr - aligned_addr)};
+  };
+
+#ifdef _WIN32
+  // PrefetchVirtualMemory() is available on Windows 8 or later
+  struct PrefetchEntry {  // Like WIN32_MEMORY_RANGE_ENTRY
+    void* VirtualAddress;
+    size_t NumberOfBytes;
+
+    PrefetchEntry(const MemoryRegion& region)  // NOLINT runtime/explicit
+        : VirtualAddress(region.addr), NumberOfBytes(region.size) {}
+  };
+  using PrefetchVirtualMemoryFunc = BOOL (*)(HANDLE, ULONG_PTR, PrefetchEntry*, ULONG);
+  static const auto prefetch_virtual_memory = reinterpret_cast<PrefetchVirtualMemoryFunc>(
+      GetProcAddress(GetModuleHandleW(L"kernel32.dll"), "PrefetchVirtualMemory"));

Review comment:
       Back to your python roots with reflection :)

##########
File path: cpp/src/arrow/io/file.cc
##########
@@ -262,6 +266,27 @@ class ReadableFile::ReadableFileImpl : public OSFile {
     return std::move(buffer);
   }
 
+  Status WillNeed(const std::vector<ReadRange>& ranges) {
+    RETURN_NOT_OK(CheckClosed());
+    for (const auto& range : ranges) {
+      RETURN_NOT_OK(internal::ValidateRange(range.offset, range.length));
+#if defined(POSIX_FADV_WILLNEED)
+      if (posix_fadvise(fd_, range.offset, range.length, POSIX_FADV_WILLNEED)) {
+        return IOErrorFromErrno(errno, "posix_fadvise failed");
+      }
+#elif defined(F_RDADVISE)  // macOS, BSD?
+      struct {
+        off_t ra_offset;
+        int ra_count;
+      } radvisory{range.offset, static_cast<int>(range.length)};

Review comment:
       Check if length <= INT_MAX, oss-fuzz is always waiting in a dark corner.  You can error/noop for now, or do the right thing and slice & re-partition.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org