From ed9b3348def2d1bb1449c836d5054d5381c9c855 Mon Sep 17 00:00:00 2001 From: Ypatia Tsavliri Date: Thu, 19 Dec 2024 13:47:09 +0200 Subject: [PATCH] Some more PR cleanup --- .../unit-sparse-unordered-with-dups-reader.cc | 12 +++++---- tiledb/sm/query/readers/filtered_data.h | 2 +- tiledb/sm/query/readers/reader_base.cc | 2 ++ tiledb/sm/query/readers/result_tile.h | 25 +++++++++++-------- tiledb/sm/tile/tile.cc | 1 - tiledb/sm/tile/tile.h | 15 +++++++---- 6 files changed, 34 insertions(+), 23 deletions(-) diff --git a/test/src/unit-sparse-unordered-with-dups-reader.cc b/test/src/unit-sparse-unordered-with-dups-reader.cc index 48aef54d30e..b11a4f0fe51 100644 --- a/test/src/unit-sparse-unordered-with-dups-reader.cc +++ b/test/src/unit-sparse-unordered-with-dups-reader.cc @@ -1065,11 +1065,13 @@ TEST_CASE_METHOD( if (one_frag) { CHECK(1 == loop_num->second); } - - // FIXME: This check has become unpredictable, see why the loop number is - // not consistent } else { - // CHECK(20 == loop_num->second); - // } + /** + * FIXME: The loop_num appears to be different on different architectures/build modes. + * SC-61065 to investigate why. + * } else { + * CHECK(20 == loop_num->second); + * } + */ // Try to read multiple frags without partial tile offset reading. Should // fail diff --git a/tiledb/sm/query/readers/filtered_data.h b/tiledb/sm/query/readers/filtered_data.h index d5a9e315add..72c29e5f8e9 100644 --- a/tiledb/sm/query/readers/filtered_data.h +++ b/tiledb/sm/query/readers/filtered_data.h @@ -415,7 +415,7 @@ class FilteredData { auto timer_se = stats_->start_timer("read"); return resources_.vfs().read(uri, offset, data, size, false); }); - // This should be changes once we use taskgraphs for modeling the data flow + // This should be changed once we use taskgraphs for modeling the data flow block.set_io_task(task); } diff --git a/tiledb/sm/query/readers/reader_base.cc b/tiledb/sm/query/readers/reader_base.cc index 0c32930c5bb..2fac0c95774 100644 --- a/tiledb/sm/query/readers/reader_base.cc +++ b/tiledb/sm/query/readers/reader_base.cc @@ -969,6 +969,7 @@ Status ReaderBase::unfilter_tiles( return Status::Ok(); } + // The current threadpool design does not allow for unfiltering to happen in chunks using a parallel for within this async task as the wait_all in the end of the parallel for can deadlock. for (uint64_t range_thread_idx = 0; range_thread_idx < num_range_threads; range_thread_idx++) { @@ -998,6 +999,7 @@ Status ReaderBase::unfilter_tiles( continue; } + // Unfiltering tasks have been launched, set the tasks to wait for in the corresponding tiles. When those tasks(futures) will be ready the tile processing that depends on the unfiltered tile will get unblocked. auto tile_tuple = result_tile->tile_tuple(name); tile_tuple->fixed_tile().set_unfilter_data_compute_task(task); diff --git a/tiledb/sm/query/readers/result_tile.h b/tiledb/sm/query/readers/result_tile.h index 0ed9d5b9e06..b762d660613 100644 --- a/tiledb/sm/query/readers/result_tile.h +++ b/tiledb/sm/query/readers/result_tile.h @@ -228,17 +228,20 @@ class ResultTile { } ~TileData() { - // TODO: destructor should not throw, catch any exceptions - if (fixed_filtered_data_task_.valid()) { - auto st = fixed_filtered_data_task_.wait(); - } - - if (var_filtered_data_task_.valid()) { - auto st = var_filtered_data_task_.wait(); - } - - if (validity_filtered_data_task_.valid()) { - auto st = validity_filtered_data_task_.wait(); + try { + if (fixed_filtered_data_task_.valid()) { + auto st = fixed_filtered_data_task_.wait(); + } + + if (var_filtered_data_task_.valid()) { + auto st = var_filtered_data_task_.wait(); + } + + if (validity_filtered_data_task_.valid()) { + auto st = validity_filtered_data_task_.wait(); + } + } catch (...) { + return; } } diff --git a/tiledb/sm/tile/tile.cc b/tiledb/sm/tile/tile.cc index df0b7c1ff4d..9d5b3badb9b 100644 --- a/tiledb/sm/tile/tile.cc +++ b/tiledb/sm/tile/tile.cc @@ -32,7 +32,6 @@ #include "tiledb/sm/tile/tile.h" -#include #include "tiledb/common/exception/exception.h" #include "tiledb/common/heap_memory.h" #include "tiledb/common/memory_tracker.h" diff --git a/tiledb/sm/tile/tile.h b/tiledb/sm/tile/tile.h index 258b23b822f..7ac9bd217ed 100644 --- a/tiledb/sm/tile/tile.h +++ b/tiledb/sm/tile/tile.h @@ -107,9 +107,11 @@ class TileBase { return static_cast(data()); } - /** Converts the data pointer to a specific type with no check on compute + /** + * Converts the data pointer to a specific type with no check on compute * task. This is used for getting thte data from inside the compute thread - * itself for unfiltering. */ + * itself for unfiltering. + */ template inline T* data_as_unsafe() const { return static_cast(data_unsafe()); @@ -134,8 +136,10 @@ class TileBase { return data_.get(); } - /** Returns the internal buffer. This is used for getting thte data from - * inside the compute thread itself for unfiltering. */ + /** + * Returns the internal buffer. This is used for getting thte data from + * inside the compute thread itself for unfiltering. + */ inline void* data_unsafe() const { return data_.get(); } @@ -198,7 +202,8 @@ class TileBase { /** The tile data type. */ Datatype type_; - /** Whether to block waiting for io data to be ready before accessing data() + /** + * Whether to block waiting for io data to be ready before accessing data() */ const bool skip_waiting_on_io_task_;