From 848a59b5bdbbb2aa949a0265ef1571d6abaafc0b Mon Sep 17 00:00:00 2001 From: Riley Dixon Date: Fri, 28 Nov 2025 13:47:58 -0700 Subject: [PATCH 1/3] Batch: Create Mock BatchOperation. --- hipfile/src/amd_detail/batch/batch.h | 7 ++++++- hipfile/test/amd_detail/mbatch.h | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/hipfile/src/amd_detail/batch/batch.h b/hipfile/src/amd_detail/batch/batch.h index f8372a17..7bd07213 100644 --- a/hipfile/src/amd_detail/batch/batch.h +++ b/hipfile/src/amd_detail/batch/batch.h @@ -28,8 +28,13 @@ struct InvalidBatchHandle : public std::invalid_argument { } }; +class IBatchOperation { +public: + virtual ~IBatchOperation() = default; +}; + /// @brief Represents a single IO Request -class BatchOperation { +class BatchOperation : public IBatchOperation { public: /// @brief Create an operation to handle and track an IO request. /// @param [in] params IO parameters diff --git a/hipfile/test/amd_detail/mbatch.h b/hipfile/test/amd_detail/mbatch.h index c59ba908..e5d21bed 100644 --- a/hipfile/test/amd_detail/mbatch.h +++ b/hipfile/test/amd_detail/mbatch.h @@ -15,6 +15,9 @@ namespace hipFile { +class MBatchOperation : public IBatchOperation { +}; + class MBatchContext : public IBatchContext { public: MOCK_METHOD(unsigned, get_capacity, (), (const, noexcept, override)); From 37ea205e06704ce420f79877f401deee63ea9108 Mon Sep 17 00:00:00 2001 From: Riley Dixon Date: Fri, 28 Nov 2025 15:29:46 -0700 Subject: [PATCH 2/3] Batch: Replace with IBatchOperation. --- hipfile/src/amd_detail/batch/batch.cpp | 4 ++-- hipfile/src/amd_detail/batch/batch.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hipfile/src/amd_detail/batch/batch.cpp b/hipfile/src/amd_detail/batch/batch.cpp index 7ff7c14f..460a7d93 100644 --- a/hipfile/src/amd_detail/batch/batch.cpp +++ b/hipfile/src/amd_detail/batch/batch.cpp @@ -113,7 +113,7 @@ BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_pa throw std::invalid_argument(msg.str()); } - std::vector> pending_ops{}; + std::vector> pending_ops{}; // It would be more performant to be able to perform multiple lookups // rather than waiting to lock the DriverState lock for each lookup. @@ -124,7 +124,7 @@ BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_pa // file flags. auto [_file, _buffer] = Context::get()->getFileAndBuffer( param_copy->fh, param_copy->u.batch.devPtr_base, param_copy->u.batch.size, 0); - auto op = std::make_shared(std::move(param_copy), _buffer, _file); + auto op = std::shared_ptr{new BatchOperation{std::move(param_copy), _buffer, _file}}; pending_ops.push_back(op); } diff --git a/hipfile/src/amd_detail/batch/batch.h b/hipfile/src/amd_detail/batch/batch.h index 7bd07213..8a691aad 100644 --- a/hipfile/src/amd_detail/batch/batch.h +++ b/hipfile/src/amd_detail/batch/batch.h @@ -94,7 +94,7 @@ class BatchContext : public IBatchContext { /// but is not yet complete or completed but not yet retrieved by the /// application. /// shared_ptr as it may need to be passed to a backend. - std::unordered_set> outstanding_ops; + std::unordered_set> outstanding_ops; BatchContext(unsigned capacity); From ca4958e42a97e1e1761a7b5a87265a68573af244 Mon Sep 17 00:00:00 2001 From: Riley Dixon Date: Fri, 5 Dec 2025 15:39:38 -0700 Subject: [PATCH 3/3] Add Factory mechanism to choose to make Real or Mock Operations This allows for unit testing to switch what kind of IBatchOperation gets made. This also explores providing a testing queue to allow a test to create and modify the mocks that get generated from the factory for additional control. Some changes here should not be included in the final PR. These have been clearly marked, and are used to verify that the factory is working as intended. --- hipfile/src/amd_detail/batch/batch.cpp | 7 +++-- hipfile/src/amd_detail/batch/batch.h | 23 +++++++++++++-- hipfile/test/amd_detail/batch/batch.cpp | 30 +++++++++++++++++++ hipfile/test/amd_detail/mbatch.h | 38 ++++++++++++++++++++++++- 4 files changed, 92 insertions(+), 6 deletions(-) diff --git a/hipfile/src/amd_detail/batch/batch.cpp b/hipfile/src/amd_detail/batch/batch.cpp index 460a7d93..59aef6e1 100644 --- a/hipfile/src/amd_detail/batch/batch.cpp +++ b/hipfile/src/amd_detail/batch/batch.cpp @@ -100,9 +100,10 @@ BatchContext::get_capacity() const noexcept } void -BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_params) +BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_params, const BatchOpMaker& make_op) { std::unique_lock _ulock{context_mutex}; + (void)default_make_op; // Check num_params first before doing anything else if (num_params > capacity - outstanding_ops.size()) { @@ -124,8 +125,8 @@ BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_pa // file flags. auto [_file, _buffer] = Context::get()->getFileAndBuffer( param_copy->fh, param_copy->u.batch.devPtr_base, param_copy->u.batch.size, 0); - auto op = std::shared_ptr{new BatchOperation{std::move(param_copy), _buffer, _file}}; - + //auto op = std::shared_ptr{new BatchOperation{std::move(param_copy), _buffer, _file}}; + auto op = make_op(std::move(param_copy), _buffer, _file); pending_ops.push_back(op); } diff --git a/hipfile/src/amd_detail/batch/batch.h b/hipfile/src/amd_detail/batch/batch.h index 8a691aad..c7aa666d 100644 --- a/hipfile/src/amd_detail/batch/batch.h +++ b/hipfile/src/amd_detail/batch/batch.h @@ -7,6 +7,7 @@ #include "hipfile.h" +#include #include #include #include @@ -55,13 +56,27 @@ class BatchOperation : public IBatchOperation { const std::shared_ptr file; }; +#include "hipfile-warnings.h" +HIPFILE_WARN_OFF("unused-function") +using BatchOpMaker = std::function(std::unique_ptr, std::shared_ptr, std::shared_ptr)>; +static const std::shared_ptr default_make_op(std::unique_ptr p, std::shared_ptr b, std::shared_ptr f) +{ + return std::dynamic_pointer_cast(std::make_shared(std::move(p), b, f)); +} +// OR +inline constexpr auto DefaultBatchOpMaker = [](std::unique_ptr p, std::shared_ptr b, std::shared_ptr f){ + return std::dynamic_pointer_cast(std::make_shared(std::move(p), b, f)); +}; +HIPFILE_WARN_ON("unused-function") + class IBatchContext { public: static constexpr unsigned MAX_SIZE = 128; virtual ~IBatchContext() = default; virtual unsigned get_capacity() const noexcept = 0; - virtual void submit_operations(const hipFileIOParams_t *params, unsigned num_params) = 0; + virtual void submit_operations(const hipFileIOParams_t *params, unsigned num_params, const BatchOpMaker& make_op = DefaultBatchOpMaker) = 0; + virtual std::unordered_set> get_ops() = 0; }; class BatchContext : public IBatchContext { @@ -81,8 +96,12 @@ class BatchContext : public IBatchContext { /// @note This is an All or None operation. If one submitted operation is not valid, no operations /// will be submitted. /// - void submit_operations(const hipFileIOParams_t *params, const unsigned num_params) override; + void submit_operations(const hipFileIOParams_t *params, const unsigned num_params, const BatchOpMaker& make_op) override; + // not a real function - PoC used to peer internally that the correct factory was used + std::unordered_set> get_ops() override { + return outstanding_ops; + } private: const unsigned capacity; diff --git a/hipfile/test/amd_detail/batch/batch.cpp b/hipfile/test/amd_detail/batch/batch.cpp index 1c1c0044..c7df4198 100644 --- a/hipfile/test/amd_detail/batch/batch.cpp +++ b/hipfile/test/amd_detail/batch/batch.cpp @@ -10,6 +10,7 @@ #include "hipfile-test.h" #include "hipfile-warnings.h" #include "invalid-enum.h" +#include "mbatch.h" #include "mbuffer.h" #include "mfile.h" #include "mstate.h" @@ -19,6 +20,7 @@ #include #include #include +#include #include #include @@ -348,4 +350,32 @@ TEST_F(HipFileBatchContext, SubmitSingleBadParamModeInvalid) ASSERT_THROW(_context->submit_operations(&bad_io_params, 1), std::invalid_argument); } +// Not a real test - testing proof of concept +TEST_F(HipFileBatchContext, _UseMockedFactory) +{ + _context->submit_operations(&io_params, 1, MBatchOperation::MBatchOpMaker); + auto context_ops = _context->get_ops(); + for(auto op : context_ops) { + // hack since we don't have the key to directly reference + //ASSERT_EQ(typeid(op.get()), typeid(MBatchOperation)); + ASSERT_NE(dynamic_cast(op.get()), nullptr); + } +} + +TEST_F(HipFileBatchContext, _UseMockedFactoryWithQueue) +{ + auto& mocked_ops = MBatchOperation::get_queue(); + std::shared_ptr m_op = std::make_shared(); + + mocked_ops.push(m_op); + ASSERT_FALSE(mocked_ops.empty()); + _context->submit_operations(&io_params, 1, MBatchOperation::MBatchOpMaker_queue); + + ASSERT_TRUE(mocked_ops.empty()); // Queue of mocks has been emptied. + + auto context_ops = _context->get_ops(); + ASSERT_EQ(context_ops.count(m_op), 1); // m_op is in the Context. +} + + HIPFILE_WARN_NO_GLOBAL_CTOR_ON diff --git a/hipfile/test/amd_detail/mbatch.h b/hipfile/test/amd_detail/mbatch.h index e5d21bed..b62dd812 100644 --- a/hipfile/test/amd_detail/mbatch.h +++ b/hipfile/test/amd_detail/mbatch.h @@ -6,9 +6,13 @@ #pragma once #include "batch/batch.h" +#include "hipfile-warnings.h" #include +#include +#include + /* * A place to create mocks for the batch module. */ @@ -16,13 +20,45 @@ namespace hipFile { class MBatchOperation : public IBatchOperation { +public: + MBatchOperation() = default; + + inline static std::queue>& get_queue() { + HIPFILE_WARN_NO_EXIT_DTOR_OFF + static std::queue> mocked_ops; + HIPFILE_WARN_NO_EXIT_DTOR_ON + return mocked_ops; + } + + inline static constexpr auto MBatchOpMaker = [](std::unique_ptr p, std::shared_ptr b, std::shared_ptr f) { + // Discard params + (void) p; + (void) b; + (void) f; + return std::make_shared(); + }; + // OR + inline static constexpr auto MBatchOpMaker_queue = [](std::unique_ptr p, std::shared_ptr b, std::shared_ptr f) { + // Discard params + (void) p; + (void) b; + (void) f; + auto& mocked_ops = get_queue(); + if (mocked_ops.empty()) { + throw std::runtime_error("Testing error: No mocks available to construct."); + } + auto op = mocked_ops.front(); + mocked_ops.pop(); + return op; + }; }; class MBatchContext : public IBatchContext { public: MOCK_METHOD(unsigned, get_capacity, (), (const, noexcept, override)); - MOCK_METHOD(void, submit_operations, (const hipFileIOParams_t *params, const unsigned num_params), + MOCK_METHOD(void, submit_operations, (const hipFileIOParams_t *params, const unsigned num_params, const BatchOpMaker& make_op), (override)); + MOCK_METHOD(std::unordered_set>, get_ops, (), (override)); }; }