From a88900a74ab34b160d3238744a6ae3ba1f5f55bc 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 0687d214a744832d23ee03406592aa6db9decbd9 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 59feb08e96edda16ae07fdc91b7877d617ad135d Mon Sep 17 00:00:00 2001 From: Riley Dixon Date: Wed, 3 Dec 2025 09:22:50 -0700 Subject: [PATCH 3/3] Incomplete: An attempt at making a Generic Factory This is not a complete solution. This was an attempt to use a templated generic factory that would match a call to make an instance to the templated constructor. This mechanism would choose the most suitable constructor if it is overloaded, or fail at compile time if no suitable constructor is found. The most obvious code smell is that in order to get this to compile, a dummy "IFactory" class needs to be inherited from. This is so that we do not need to declare the implementation of BatchOperation in the submit_operations() signature. IFactory is empty and does not actually contain the functions we are interested in. Those instead lie with the GenericFactory which can't be listed in the signature as GenericFactory needs to know the implementation. I am leaving this here in case anyone wants to take a peek at the diff and has an idea if this is worthwhile to pursue. --- hipfile/src/amd_detail/batch/batch.cpp | 86 ++++++++++++++++++++++++- hipfile/src/amd_detail/batch/batch.h | 89 +++++++++++++++++++++++++- hipfile/test/amd_detail/mbatch.h | 2 +- 3 files changed, 173 insertions(+), 4 deletions(-) diff --git a/hipfile/src/amd_detail/batch/batch.cpp b/hipfile/src/amd_detail/batch/batch.cpp index 460a7d93..52f1f10a 100644 --- a/hipfile/src/amd_detail/batch/batch.cpp +++ b/hipfile/src/amd_detail/batch/batch.cpp @@ -22,6 +22,72 @@ namespace hipFile { +// /* +// Factory was created with the help of an LLM +// - typename... Args - "parameter pack" of zero or more types +// - Args... - Expand the "parameter pack" +// - typename = std::enable_if_t<...> - dummy template param used as a gate +// used to check for function resolution +// Allows for trying all overloads. +// If no matching signature found at all: compiler error. +// - forward(args)... - "perfectly forward" all passed args into T's matched ctor. +// aka. expand the arg type pack and arg value pack into +// separate std::forward calls. +// */ +// template +// struct GenericFactory { + +// // Return an instance of T +// // Usage: Factory::make(...) +// template < +// typename... Args, +// typename = std::enable_if_t::value> +// > +// static T make(Args&&... args) +// { +// return T(std::forward(args)...); +// } + +// // Return a std::shared_ptr of T +// // Usage:: Factory::make_shared(...) +// template < +// typename... Args, +// typename = std::enable_if_t::value> +// > +// static std::shared_ptr make_shared(Args&&... args) +// { +// return std::make_shared(std::forward(args)...); +// } + +// // Return a std::shared_ptr of a base class of T +// // Usage: Factory::make_shared_as(...) +// template < +// typename BaseT, +// typename... Args, +// typename = std::enable_if_t< +// std::is_constructible::value && +// std::is_base_of::value +// > +// > +// static std::shared_ptr make_shared_as(Args&&... args) +// { +// return std::static_pointer_cast(std::make_shared(std::forward(args)...)); +// //return std::shared_ptr{new T{std::forward(std::forward(args)...); +// } + +// // Return a std::unique_ptr of T +// template < +// typename... Args, +// typename = std::enable_if_t::value> +// > +// static std::unique_ptr make_unique(Args&&... args) +// { +// return std::make_unique(std::forward(args)...); +// } +// }; + + BatchOperation::BatchOperation(std::unique_ptr params, std::shared_ptr _buffer, std::shared_ptr _file) : io_params{std::move(params)}, buffer{_buffer}, file{_file} @@ -99,9 +165,26 @@ BatchContext::get_capacity() const noexcept return capacity; } +//using BatchOpFactory = std::function + +// template +// void +// test_usage(Factory& fac) +// { +// (void)fac; +// } + +// void +// test_caller() +// { +// Factory fac; +// test_usage(fac); +// } + void -BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_params) +BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_params, IFactory factory) { + (void)factory; std::unique_lock _ulock{context_mutex}; // Check num_params first before doing anything else @@ -126,6 +209,7 @@ BatchContext::submit_operations(const hipFileIOParams_t *params, unsigned num_pa 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}}; + 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..c75610f2 100644 --- a/hipfile/src/amd_detail/batch/batch.h +++ b/hipfile/src/amd_detail/batch/batch.h @@ -22,6 +22,81 @@ class IFile; namespace hipFile { +/* + * Intentionally empty interface + */ +template +class IFactory { +}; + +/* + Factory was created with the help of an LLM + - typename... Args - "parameter pack" of zero or more types + - Args... - Expand the "parameter pack" + - typename = std::enable_if_t<...> - dummy template param used as a gate + used to check for function resolution + Allows for trying all overloads. + If no matching signature found at all: compiler error. + - forward(args)... - "perfectly forward" all passed args into T's matched ctor. + aka. expand the arg type pack and arg value pack into + separate std::forward calls. +*/ +template < + typename Interface, + typename Implementation +> +class GenericFactory { +public: + // Return an instance of Implementation + // Usage: Factory::make(...) + template < + typename... Args, + typename = std::enable_if_t::value> + > + static Implementation make(Args&&... args) + { + return Implementation(std::forward(args)...); + } + + // Return a std::shared_ptr to an Implementation instance + // Usage:: Factory::make_shared(...) + template < + typename... Args, + typename = std::enable_if_t::value> + > + static std::shared_ptr make_shared(Args&&... args) + { + return std::static_pointer_cast(std::make_shared(std::forward(args)...)); + } + + // // Return a std::shared_ptr to an arbitrary + // // Usage: Factory::make_shared_as(...) + // template < + // typename BaseT, + // typename... Args, + // typename = std::enable_if_t< + // std::is_constructible::value && + // std::is_base_of::value + // > + // > + // static std::shared_ptr make_shared_as(Args&&... args) + // { + // return std::static_pointer_cast(std::make_shared(std::forward(args)...)); + // //return std::shared_ptr{new T{std::forward(std::forward(args)...); + // } + + // // Return a std::unique_ptr of T + // template < + // typename... Args, + // typename = std::enable_if_t::value> + // > + // static std::unique_ptr make_unique(Args&&... args) + // { + // return std::make_unique(std::forward(args)...); + // } +}; + struct InvalidBatchHandle : public std::invalid_argument { InvalidBatchHandle() : std::invalid_argument{"Invalid batch handle"} { @@ -55,13 +130,21 @@ class BatchOperation : public IBatchOperation { const std::shared_ptr file; }; +template < + typename Implementation, + typename Interface = IBatchOperation +> +class BatchOperationFactory : public IFactory, public GenericFactory +{ +}; + 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, IFactory factory = BatchOperationFactory{}) = 0; }; class BatchContext : public IBatchContext { @@ -81,7 +164,7 @@ 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, IFactory factory) override; private: const unsigned capacity; @@ -136,4 +219,6 @@ class BatchContextMap { mutable std::shared_mutex batch_mutex; }; +static_assert(!std::is_abstract>::value, "Not concrete"); + } diff --git a/hipfile/test/amd_detail/mbatch.h b/hipfile/test/amd_detail/mbatch.h index e5d21bed..a4e799db 100644 --- a/hipfile/test/amd_detail/mbatch.h +++ b/hipfile/test/amd_detail/mbatch.h @@ -21,7 +21,7 @@ class MBatchOperation : public IBatchOperation { 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, IFactory factory), (override)); };