From b68a6c7a1dca85765097ba37910d032455d9e19f Mon Sep 17 00:00:00 2001
From: Yoav Helfman <ifdef@meta.com>
Date: Fri, 21 Jun 2024 18:43:12 -0700
Subject: [PATCH] Fix IOStats for Nimble (#65)

Summary:
Pull Request resolved: https://github.com/facebookincubator/nimble/pull/65

X-link: https://github.com/facebookincubator/velox/pull/10216

IOStats are being calculated in different layers of the IO stacks.
Since Nimble and DWRF don't share parts of the stack, some IOStats calculation were not affecting Nimble.

Probably the right thing to do is to move all IOStats calculations to the bottom layers (WSFile, cache and SSD reads), where IO is actually performed (and these layers are shared beteen Nimble nad DWRF).
But it seems like that for this change, we need a design, clarifying what we actually want to track and how to track it.

Since we don't have the cycles to create this design right now, I opted for a simple solution, where I create a simple layer on the Nimble side, which will calculate these stats.

Reviewed By: Yuhta, sdruzkin

Differential Revision: D58559606
---
 dwio/nimble/common/tests/TestUtils.h |  6 +++++-
 dwio/nimble/tablet/TabletReader.cpp  | 11 +----------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/dwio/nimble/common/tests/TestUtils.h b/dwio/nimble/common/tests/TestUtils.h
index 9cd14ecc..3c1bb15c 100644
--- a/dwio/nimble/common/tests/TestUtils.h
+++ b/dwio/nimble/common/tests/TestUtils.h
@@ -307,12 +307,14 @@ class InMemoryTrackableReadFile final : public velox::ReadFile {
     NIMBLE_NOT_SUPPORTED("Not used by Nimble");
   }
 
-  void preadv(
+  uint64_t preadv(
       folly::Range<const velox::common::Region*> regions,
       folly::Range<folly::IOBuf*> iobufs) const override {
     VELOX_CHECK_EQ(regions.size(), iobufs.size());
+    uint64_t length = 0;
     for (size_t i = 0; i < regions.size(); ++i) {
       const auto& region = regions[i];
+      length += region.length;
       auto& output = iobufs[i];
       if (shouldProduceChainedBuffers_) {
         chunks_.wlock()->push_back({region.offset, region.length});
@@ -332,6 +334,8 @@ class InMemoryTrackableReadFile final : public velox::ReadFile {
         output.append(region.length);
       }
     }
+
+    return length;
   }
 
   uint64_t size() const final {
diff --git a/dwio/nimble/tablet/TabletReader.cpp b/dwio/nimble/tablet/TabletReader.cpp
index b32f1c1a..ce1c56da 100644
--- a/dwio/nimble/tablet/TabletReader.cpp
+++ b/dwio/nimble/tablet/TabletReader.cpp
@@ -33,16 +33,6 @@
 
 namespace facebook::nimble {
 
-DEFINE_bool(
-    nimble_disable_coalesce,
-    false,
-    "Disable read coalescing in Nimble reader.");
-
-DEFINE_uint64(
-    nimble_coalesce_max_distance,
-    1024 * 1024 * 1.25,
-    "Maximum read coalescing distance in Nimble reader. And gap smaller than this value will be coalesced.");
-
 // Here's the layout of the tablet:
 //
 // stripe 1 streams
@@ -59,6 +49,7 @@ DEFINE_uint64(
 // 2 bytes major version + 2 bytes minor version +
 // 4 bytes magic number.
 namespace {
+
 template <typename T>
 const T* asFlatBuffersRoot(std::string_view content) {
   return flatbuffers::GetRoot<T>(content.data());