From 4723950b0bd19db7b92707c3d2fd7a8d01d440b5 Mon Sep 17 00:00:00 2001
From: Ypatia Tsavliri <ypatia@tiledb.com>
Date: Wed, 18 Dec 2024 13:28:03 +0200
Subject: [PATCH] Cleaup PR

---
 tiledb/sm/tile/tile.cc |  8 ++++----
 tiledb/sm/tile/tile.h  | 36 ++++++++----------------------------
 2 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/tiledb/sm/tile/tile.cc b/tiledb/sm/tile/tile.cc
index 9b05871ec60..df0b7c1ff4d 100644
--- a/tiledb/sm/tile/tile.cc
+++ b/tiledb/sm/tile/tile.cc
@@ -112,14 +112,14 @@ TileBase::TileBase(
     const uint64_t cell_size,
     const uint64_t size,
     tdb::pmr::memory_resource* resource,
-    const bool ignore_tasks)
+    const bool skip_waiting_on_io_task)
     : resource_(resource)
     , data_(tdb::pmr::make_unique<std::byte>(resource_, size))
     , size_(size)
     , cell_size_(cell_size)
     , format_version_(format_version)
     , type_(type)
-    , ignore_tasks_(ignore_tasks) {
+    , skip_waiting_on_io_task_(skip_waiting_on_io_task) {
   /*
    * We can check for a bad allocation after initialization without risk
    * because none of the other member variables use its value for their own
@@ -211,7 +211,7 @@ WriterTile::WriterTile(
 
 void TileBase::read(
     void* const buffer, const uint64_t offset, const uint64_t nbytes) const {
-  if (!ignore_tasks_) {
+  if (!skip_waiting_on_io_task_) {
     if (unfilter_data_compute_task_.valid()) {
       throw_if_not_ok(unfilter_data_compute_task_.wait());
     } else {
@@ -312,7 +312,7 @@ void WriterTile::write_var(const void* data, uint64_t offset, uint64_t nbytes) {
 
 uint64_t Tile::load_chunk_data(
     ChunkData& unfiltered_tile, uint64_t expected_original_size) {
-  // std::scoped_lock<std::recursive_mutex> lock{filtered_data_io_task_mtx_};
+  std::scoped_lock<std::recursive_mutex> lock{filtered_data_io_task_mtx_};
   assert(filtered());
 
   Deserializer deserializer(filtered_data(), filtered_size());
diff --git a/tiledb/sm/tile/tile.h b/tiledb/sm/tile/tile.h
index 7e344917ee6..c7608a7b65e 100644
--- a/tiledb/sm/tile/tile.h
+++ b/tiledb/sm/tile/tile.h
@@ -62,7 +62,9 @@ class TileBase {
    * @param cell_size The cell size.
    * @param size The size of the tile.
    * @param resource The memory resource to use.
-   * @param data_io_task The I/O task to wait on for data to be valid.
+   * @param skip_waiting_on_io_task whether to skip waiting on I/O tasks and
+   * directly access data() or block. By default is false, so by default we
+   * block waiting. Used when we create generic tiles or in testing.
    */
   TileBase(
       const format_version_t format_version,
@@ -70,18 +72,11 @@ class TileBase {
       const uint64_t cell_size,
       const uint64_t size,
       tdb::pmr::memory_resource* resource,
-      const bool ignore_tasks = false);
+      const bool skip_waiting_on_io_task = false);
 
   DISABLE_COPY_AND_COPY_ASSIGN(TileBase);
   DISABLE_MOVE_AND_MOVE_ASSIGN(TileBase);
 
-  ~TileBase() {
-    // TODO: destructor should not throw, catch any exceptions
-    if (unfilter_data_compute_task_.valid()) {
-      auto st = unfilter_data_compute_task_.wait();
-    }
-  }
-
   /* ********************************* */
   /*                API                */
   /* ********************************* */
@@ -118,7 +113,7 @@ class TileBase {
 
   /** Returns the internal buffer. */
   inline void* data() const {
-    if (!ignore_tasks_) {
+    if (!skip_waiting_on_io_task_) {
       if (unfilter_data_compute_task_.valid()) {
         throw_if_not_ok(unfilter_data_compute_task_.wait());
       } else {
@@ -193,18 +188,12 @@ class TileBase {
   /** The tile data type. */
   Datatype type_;
 
-  const bool ignore_tasks_;
+  /** Whether to block waiting for io data to be ready before accessing data()
+   */
+  const bool skip_waiting_on_io_task_;
 
   /** Compute task to check and block on if unfiltered data is ready. */
   mutable ThreadPool::SharedTask unfilter_data_compute_task_;
-
-  /**
-   * Lock for checking task, since this tile can be used by multiple threads.
-   * The ThreadPool::SharedTask lets multiple threads copy the task, but it
-   * doesn't let multiple threads access a single task itself. Due to this we
-   * need a mutext since the tile will be accessed by multiple threads.
-   */
-  mutable std::recursive_mutex unfilter_data_compute_task_mtx_;
 };
 
 /**
@@ -284,13 +273,6 @@ class Tile : public TileBase {
   DISABLE_MOVE_AND_MOVE_ASSIGN(Tile);
   DISABLE_COPY_AND_COPY_ASSIGN(Tile);
 
-  ~Tile() {
-    // TODO: destructor should not throw, catch any exceptions
-    if (unfilter_data_compute_task_.valid()) {
-      auto st = unfilter_data_compute_task_.wait();
-    }
-  }
-
   /* ********************************* */
   /*                API                */
   /* ********************************* */
@@ -519,7 +501,6 @@ class WriterTile : public TileBase {
    * @param cell_size The cell size.
    * @param size The size of the tile.
    * @param memory_tracker The memory tracker to use.
-   * @param data_io_task The I/O task to wait on for data to be valid.
    */
   WriterTile(
       const format_version_t format_version,
@@ -536,7 +517,6 @@ class WriterTile : public TileBase {
    * @param cell_size The cell size.
    * @param size The size of the tile.
    * @param resource The memory resource to use.
-   * @param data_io_task The I/O task to wait on for data to be valid.
    */
   WriterTile(
       const format_version_t format_version,