From 47d89446e20dbea820d4e7bb3f78249a7861c1c6 Mon Sep 17 00:00:00 2001 From: kferjaoui Date: Thu, 1 Oct 2026 15:17:03 +0200 Subject: [PATCH] CUDA: C++17 cleanup of ClusterFinderCUDA - std::array for per-slot state; std::exchange in BatchView::steal - [[nodiscard]]/noexcept on the public API and release path - cuda_check moved to aare::cuda::detail (was reserved __cuda_check) - _v type traits; drop redundant std::move and `using namespace aare` - Add pytest for BatchView slot release --- include/aare/ClusterFinderCUDA.hpp | 64 ++++++++--------- include/aare/clusterfinder_algo.cuh | 16 ++--- include/aare/utils/cuda_check.cuh | 8 ++- python/src/bind_ClusterFinderCUDA.hpp | 4 +- python/src/bind_ClusterFinderCUDAGraph.hpp | 4 +- python/tests/test_ClusterFinderCUDA.py | 80 ++++++++++++++++++++++ 6 files changed, 128 insertions(+), 48 deletions(-) create mode 100644 python/tests/test_ClusterFinderCUDA.py diff --git a/include/aare/ClusterFinderCUDA.hpp b/include/aare/ClusterFinderCUDA.hpp index 3ee75bf8..dd78fded 100644 --- a/include/aare/ClusterFinderCUDA.hpp +++ b/include/aare/ClusterFinderCUDA.hpp @@ -5,6 +5,7 @@ #include "aare/clusterfinder_algo.cuh" #include "aare/utils/cuda_check.cuh" #include +#include #include #include #include @@ -12,6 +13,7 @@ #include #include #include +#include #include namespace aare::cuda { @@ -104,18 +106,18 @@ class ClusterFinderCUDA { // separate slots. A third submit() without an intervening collect() throws. static constexpr int NUM_SLOTS = 2; - void *h_output_slots[NUM_SLOTS] = {nullptr, nullptr}; - size_t m_output_slot_capacity[NUM_SLOTS] = {0, 0}; + std::array h_output_slots{}; + std::array m_output_slot_capacity{}; // Per-slot state consumed by collect() - bool m_slot_in_flight[NUM_SLOTS] = {false, false}; + std::array m_slot_in_flight{}; // A BatchView handed out by collect_view() still points into this slot's // pinned buffer. The slot must not be reused until the view is released, so // submit_batch() refuses it rather than overwriting live results. - bool m_slot_view_held[NUM_SLOTS] = {false, false}; - size_t m_slot_n_frames[NUM_SLOTS] = {0, 0}; - uint64_t m_slot_first_frame[NUM_SLOTS] = {0, 0}; - int m_slot_streams_used[NUM_SLOTS] = {0, 0}; + std::array m_slot_view_held{}; + std::array m_slot_n_frames{}; + std::array m_slot_first_frame{}; + std::array m_slot_streams_used{}; int m_next_slot = 0; // Pointer registered via cudaHostRegister by the caller (for pinned H2D @@ -160,14 +162,14 @@ class ClusterFinderCUDA { // Per-slot kernel timing event pools (sized lazily to the largest batch). // Left empty when m_time_kernels is false — no events are ever created. - std::vector m_kernel_start_pools[NUM_SLOTS]; - std::vector m_kernel_stop_pools[NUM_SLOTS]; + std::array, NUM_SLOTS> m_kernel_start_pools; + std::array, NUM_SLOTS> m_kernel_stop_pools; // Per-slot, per-stream "batch done" sync events (timing disabled). // Recorded after the last D2H of each submit_batch(). collect() waits on // these via cudaEventSynchronize so it unblocks as soon as the batch // finishes, even if the next batch is already queued in the same streams. - std::vector m_batch_done[NUM_SLOTS]; + std::array, NUM_SLOTS> m_batch_done; /// Copy every frame of a pinned output slot into results, turning the /// packed device layout into owned ClusterVectors. This is exactly the work @@ -253,7 +255,7 @@ class ClusterFinderCUDA { } /// Called by BatchView when it is released, making the slot reusable. - void release_slot(int slot) { + void release_slot(int slot) noexcept { if (slot >= 0 && slot < NUM_SLOTS) m_slot_view_held[slot] = false; } @@ -346,7 +348,7 @@ class ClusterFinderCUDA { ~BatchView() { release(); } /// Give the slot back. Idempotent; the view is unusable afterwards. - void release() { + void release() noexcept { if (m_owner) m_owner->release_slot(m_slot); m_owner = nullptr; @@ -355,22 +357,24 @@ class ClusterFinderCUDA { m_slot = -1; } - bool valid() const { return m_base != nullptr; } - size_t n_frames() const { return m_n_frames; } - uint64_t first_frame() const { return m_first_frame; } + [[nodiscard]] bool valid() const noexcept { return m_base != nullptr; } + [[nodiscard]] size_t n_frames() const noexcept { return m_n_frames; } + [[nodiscard]] uint64_t first_frame() const noexcept { + return m_first_frame; + } - uint32_t count(size_t i) const { + [[nodiscard]] uint32_t count(size_t i) const { check(i); return Output::host_count(m_base + i * m_stride, static_cast(m_max_clusters)); } - const ClusterType *clusters(size_t i) const { + [[nodiscard]] const ClusterType *clusters(size_t i) const { check(i); return Output::host_clusters(m_base + i * m_stride); } - size_t total_clusters() const { + [[nodiscard]] size_t total_clusters() const { size_t n = 0; for (size_t i = 0; i < m_n_frames; ++i) n += count(i); @@ -380,7 +384,7 @@ class ClusterFinderCUDA { /// Per-cluster sums for the whole batch, concatenated frame by frame. /// The common reduction, done here so callers never have to materialise /// the clusters themselves. - std::vector sums() const { + [[nodiscard]] std::vector sums() const { std::vector out; out.reserve(total_clusters()); for (size_t i = 0; i < m_n_frames; ++i) { @@ -393,17 +397,14 @@ class ClusterFinderCUDA { } private: - void steal(BatchView &o) { - m_owner = o.m_owner; - m_base = o.m_base; + void steal(BatchView &o) noexcept { + m_owner = std::exchange(o.m_owner, nullptr); + m_base = std::exchange(o.m_base, nullptr); + m_slot = std::exchange(o.m_slot, -1); m_n_frames = o.m_n_frames; m_first_frame = o.m_first_frame; m_stride = o.m_stride; m_max_clusters = o.m_max_clusters; - m_slot = o.m_slot; - o.m_owner = nullptr; - o.m_base = nullptr; - o.m_slot = -1; } void check(size_t i) const { if (!m_base) @@ -617,7 +618,7 @@ class ClusterFinderCUDA { * @brief Move clusters out of the internal ClusterVector, optionally * reallocating the internal one with the same capacity. */ - ClusterVector + [[nodiscard]] ClusterVector steal_clusters(bool realloc_same_capacity = false) { ClusterVector tmp = std::move(m_clusters); if (realloc_same_capacity) @@ -662,8 +663,8 @@ class ClusterFinderCUDA { * } * process(cf.collect(tok)); // drain final batch */ - BatchToken submit_batch(NDView frames, - uint64_t first_frame = 0) { + [[nodiscard]] BatchToken submit_batch(NDView frames, + uint64_t first_frame = 0) { if (m_pedestal_dirty) { sync_pedestal_to_device(); m_pedestal_dirty = false; @@ -744,7 +745,8 @@ class ClusterFinderCUDA { * concurrently running batch already queued in the same streams is not * waited on — the GPU keeps running while the CPU drains this batch. */ - std::vector> collect(BatchToken token) { + [[nodiscard]] std::vector> + collect(BatchToken token) { const int slot = token.slot; if (!m_slot_in_flight[slot]) throw std::runtime_error( @@ -788,7 +790,7 @@ class ClusterFinderCUDA { * released, so at most NUM_SLOTS - 1 further batches can be submitted while * it is alive. Consume, then release. */ - BatchView collect_view(BatchToken token) { + [[nodiscard]] BatchView collect_view(BatchToken token) { const int slot = token.slot; size_t n_frames = 0; uint64_t first_frame = 0; diff --git a/include/aare/clusterfinder_algo.cuh b/include/aare/clusterfinder_algo.cuh index 511de96a..2ca504de 100644 --- a/include/aare/clusterfinder_algo.cuh +++ b/include/aare/clusterfinder_algo.cuh @@ -70,23 +70,23 @@ template struct require_algo { using Output = typename Algo::Output; using Buffers = typename Ped::Buffers; - static_assert(std::is_move_constructible::value && - !std::is_copy_constructible::value, + static_assert(std::is_move_constructible_v && + !std::is_copy_constructible_v, "Algo::Output owns device memory: it must be move-only."); - static_assert(std::is_move_constructible::value && - !std::is_copy_constructible::value, + static_assert(std::is_move_constructible_v && + !std::is_copy_constructible_v, "Ped::Buffers owns device memory: it must be move-only."); - static_assert(std::is_trivially_copyable::value, + static_assert(std::is_trivially_copyable_v, "Ped::View is passed to a kernel: it must be a trivially " "copyable bundle of pointers, owning nothing."); static_assert( - std::is_same::value && - std::is_same::value, + std::is_same_v && + std::is_same_v, "Algo::Output must expose the block layout as 'static constexpr size_t " "clusters_offset' and 'static size_t bytes_per_frame(uint32_t)': the " "driver sizes its pinned host slots from them, before any Output " "exists."); - static_assert(std::is_floating_point::value, + static_assert(std::is_floating_point_v, "Algo::compute_type is the nSigma threshold type: an " "integral type compiles and truncates nSigma (3.5 -> 3)."); diff --git a/include/aare/utils/cuda_check.cuh b/include/aare/utils/cuda_check.cuh index 424fe017..36a73864 100644 --- a/include/aare/utils/cuda_check.cuh +++ b/include/aare/utils/cuda_check.cuh @@ -3,7 +3,8 @@ #include #include -inline void __cuda_check(cudaError_t err, const char *file, int line) { +namespace aare::cuda::detail { +inline void cuda_check(cudaError_t err, const char *file, int line) { if (err != cudaSuccess) { throw std::runtime_error((std::ostringstream{} << "[CUDA ERROR] " << cudaGetErrorString(err) @@ -11,5 +12,6 @@ inline void __cuda_check(cudaError_t err, const char *file, int line) { .str()); } } - -#define CUDA_CHECK(stmt) __cuda_check((stmt), __FILE__, __LINE__) \ No newline at end of file +} // namespace aare::cuda::detail +#define CUDA_CHECK(stmt) \ + ::aare::cuda::detail::cuda_check((stmt), __FILE__, __LINE__) \ No newline at end of file diff --git a/python/src/bind_ClusterFinderCUDA.hpp b/python/src/bind_ClusterFinderCUDA.hpp index 4b81a857..91883c00 100644 --- a/python/src/bind_ClusterFinderCUDA.hpp +++ b/python/src/bind_ClusterFinderCUDA.hpp @@ -14,8 +14,6 @@ namespace py = pybind11; using pd_type = double; -using namespace aare; - #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-parameter" @@ -170,7 +168,7 @@ sqrt(max(sum2/n - mean^2, 0)). Counterpart to `noise` for the device pedestal.)" .def( "steal_clusters", [](CF &self, bool realloc_same_capacity) { - return std::move(self.steal_clusters(realloc_same_capacity)); + return self.steal_clusters(realloc_same_capacity); }, py::arg("realloc_same_capacity") = true) diff --git a/python/src/bind_ClusterFinderCUDAGraph.hpp b/python/src/bind_ClusterFinderCUDAGraph.hpp index 972419b7..d82517e1 100644 --- a/python/src/bind_ClusterFinderCUDAGraph.hpp +++ b/python/src/bind_ClusterFinderCUDAGraph.hpp @@ -13,8 +13,6 @@ namespace py = pybind11; using pd_type = double; -using namespace aare; - #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-parameter" @@ -65,7 +63,7 @@ void define_ClusterFinderCUDAGraph(py::module &m, const std::string &typestr) { .def( "steal_clusters", [](CF &self, bool realloc_same_capacity) { - return std::move(self.steal_clusters(realloc_same_capacity)); + return self.steal_clusters(realloc_same_capacity); }, py::arg("realloc_same_capacity") = true) diff --git a/python/tests/test_ClusterFinderCUDA.py b/python/tests/test_ClusterFinderCUDA.py new file mode 100644 index 00000000..defd8e0a --- /dev/null +++ b/python/tests/test_ClusterFinderCUDA.py @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: MPL-2.0 +import numpy as np +import pytest + +from aare import ClusterFinderCUDA, _cuda_available, find_cluster_views_batched_iter + +pytestmark = pytest.mark.skipif( + not _cuda_available(), reason="aare built without AARE_CUDA") + +SHAPE = (32, 32) +BATCH = 4 +# Pixel that gets a photon in every data frame, away from the border. +HIT = (16, 16) + + +@pytest.fixture +def cf(): + cf = ClusterFinderCUDA(SHAPE, n_streams=2, min_pedestal_samples=10) + rng = np.random.default_rng(42) + for _ in range(20): + cf.push_pedestal_frame( + rng.normal(1000, 5, SHAPE).astype(np.uint16)) + return cf + + +@pytest.fixture +def frames(): + rng = np.random.default_rng(7) + f = rng.normal(1000, 5, (BATCH, *SHAPE)).astype(np.uint16) + f[:, HIT[0], HIT[1]] = 3000 + return f + + +def test_collect_view_exposes_batch(cf, frames): + tok = cf.submit_batch(frames, first_frame=100) + with cf.collect_view(tok) as v: + assert v.valid + assert v.n_frames == BATCH + assert v.first_frame == 100 + assert all(v.count(i) >= 1 for i in range(BATCH)) + assert not v.valid + + +def test_released_view_frees_its_slot(cf, frames): + # Cycles both slots twice. pybind11 move-constructs every returned + # BatchView, so a move that drops the slot index leaves the slot held + # forever and the third submit_batch() throws. + for i in range(2 * 2): + tok = cf.submit_batch(frames, first_frame=i * BATCH) + v = cf.collect_view(tok) + v.release() + assert not v.valid + + +def test_dropped_view_frees_its_slot(cf, frames): + # The destructor releases, so a view that is never bound is harmless. + for i in range(2 * 2): + tok = cf.submit_batch(frames, first_frame=i * BATCH) + cf.collect_view(tok) + + +def test_held_view_blocks_its_slot(cf, frames): + held = cf.collect_view(cf.submit_batch(frames)) + # The other slot is still free. + with cf.collect_view(cf.submit_batch(frames)): + pass + with pytest.raises(RuntimeError, match="held by a BatchView"): + cf.submit_batch(frames) + held.release() + with cf.collect_view(cf.submit_batch(frames)): + pass + + +def test_views_batched_iter_pipelines_all_chunks(cf, frames): + data = np.concatenate([frames] * 3) + seen = [] + for v in find_cluster_views_batched_iter(cf, data, first_frame=10, + chunk=BATCH): + seen.append((v.first_frame, v.n_frames)) + assert seen == [(10, BATCH), (10 + BATCH, BATCH), (10 + 2 * BATCH, BATCH)]