NanoVDB: add cuda::Buffer, cuda::BufferView, and the synchronous resource concept (CUDA) - #2251
NanoVDB: add cuda::Buffer, cuda::BufferView, and the synchronous resource concept (CUDA)#2251harrism wants to merge 1 commit into
Conversation
…urce trait (CUDA) Adds nanovdb/cuda/Buffer.h with two typed containers: - cuda::Buffer<T, R>: owning, move-only container of T elements allocated from a memory resource R held by value. With a stream-ordered resource (is_async_resource) it retains the stream of the most recent allocation and orders deallocation, resize, and copy on it; with a synchronous resource (is_resource) it carries no stream state and exposes no stream API, enforced via an empty-base StreamHolder and enable_if-gated overloads. Constructors value-initialize elements to zero unless the NoInit tag is passed. Element counts whose byte size would overflow size_t are rejected with an exception. - cuda::BufferView<T>: non-owning, trivially copyable view with span semantics plus a detaching clear(), which is what the GridHandle buffer static interface requires, enabling zero-copy GridHandle<cuda::BufferView<std::byte>> over externally owned grids. Also adds the is_resource detection trait (synchronous allocate/deallocate) to nanovdb/cuda/DeviceResource.h alongside is_async_resource, and a new unit-test suite (TestBuffer.cu, ctest name nanovdb_cuda_buffer_unit_test) covering traits, counting and stream-recording resources, stream retention, resize, move semantics, deep copy, pinned-host buffers, and a zero-copy GridHandle round trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
kmuseth
left a comment
There was a problem hiding this comment.
looks good - only non-blocking changes
how much VRAM does the unit-test require? I've recently notice that tests would fail my my Nvidia laptop with only 8GB.
| /// @param count number of elements | ||
| template<typename S = R, std::enable_if_t<is_async_resource<S>::value, int> = 0> | ||
| explicit Buffer(cudaStream_t stream, R resource, size_t count, NoInit) | ||
| : detail::StreamHolder<true>{stream}, mResource(std::move(resource)) |
There was a problem hiding this comment.
very minor but I prefer:
explicit Buffer(cudaStream_t stream, R resource, size_t count, NoInit)
: detail::StreamHolder<true>{stream}
, mResource(std::move(resource))
{
I see you use the same layout below
| /// @brief Convenience c-tor using a default-constructed resource. | ||
| template<typename S = R, std::enable_if_t<!is_async_resource<S>::value && is_resource<S>::value, int> = 0> | ||
| Buffer(size_t count, NoInit) : Buffer(R(), count, noInit) {} | ||
|
|
There was a problem hiding this comment.
add:
/// Explicitly disallow copy construction and assignment operation
| /// @brief Returns the retained stream, i.e. the stream the buffer's memory | ||
| /// will be freed on. | ||
| template<typename S = R, std::enable_if_t<is_async_resource<S>::value, int> = 0> | ||
| cudaStream_t stream() const { return this->mStream; } |
There was a problem hiding this comment.
is there a reason this is not called getStream()? I see setStream(..) below so it seems a little strange that the get method uses a different semantic.
kmuseth
left a comment
There was a problem hiding this comment.
The async resize wraps its cudaMemcpyAsync in a try/catch to deallocate newData on failure, leaving the buffer untouched. The synchronous overload (Buffer.h:228-237) has no such guard:
| /// initialization of freshly allocated memory costs a hidden fill | ||
| /// pass that the dominant allocate-then-overwrite pattern wastes, | ||
| /// so initialization is always explicit (matching cuda::buffer, | ||
| /// whose count c-tor likewise requires cuda::no_init). |
There was a problem hiding this comment.
please use "@param count" etc
| } | ||
|
|
||
| /// @brief Resizes the buffer to @c count elements through the synchronous | ||
| /// resource, preserving the leading min(old, new) elements. |
|
|
||
| /// @brief Allocates @c count elements through the resource and records the | ||
| /// new extent; @c stream is used by the stream-ordered form and | ||
| /// ignored by the synchronous one. A zero count allocates nothing. |
There was a problem hiding this comment.
instead of documenting all the arguments in @brief please use "@param count" etc
| /// @brief Resizes the buffer to @c count elements through the synchronous | ||
| /// resource, preserving the leading min(old, new) elements. | ||
| template<typename S = R, std::enable_if_t<!is_async_resource<S>::value && is_resource<S>::value, int> = 0> | ||
| void resize(size_t count) |
There was a problem hiding this comment.
The async resize wraps its cudaMemcpyAsync in a try/catch to deallocate newData on failure, leaving the buffer untouched. The synchronous overload (Buffer.h:228-237) has no such guard. If cudaCheck throws (e.g. cudaErrorInvalidValue), newData is leaked and the old block is never freed.
Step 2 (part A) of #2232 (NanoVDB injectable CUDA memory resources). New types and tests only — no existing call site changes; the scratch retrofit that consumes these lands separately.
What
cuda::Buffer<T, R>— typed, resource-aware, stream-ordered container modeled on CCCL'scuda::buffer<T>. Constructor order and shape followcuda::bufferexactly —(stream, resource, count, noInit), with defaulted-resource conveniences and stream-less synchronous forms.size()counts elements,size_bytes()bytes. Move-only with an explicitcopy(). The retained-stream rule throughout: frees are ordered on the stream of the last allocation (orsetStream), andresizeorders everything — including the free of the old block, whose last use is the prefix copy — on the passed stream.cuda::BufferView<T>— typed non-owning view with span semantics (element constness inT, shallow const, trivially copyable, no resource, no stream) satisfying the same static interface, soGridHandle<BufferView<std::byte>>wraps externally owned memory with zero copies (tested end-to-end against acreateLevelSetSpheregrid — pointer-equal, no double free).is_resource<R>(synchronous tier) besideis_async_resource<R>, which now enforces CCCL's refinement: an async resource provides the stream-ordered and the synchronous interface, sois_async_resource<R>impliesis_resource<R>.DeviceResourcegains the delegating sync pair;PinnedResourceremains synchronous-only.DeviceResource's legacy staticallocateAsync/deallocateAsyncare[[deprecated]](the implementation moved into the instance methods, so nothing internal trips the warning). Migration note for v13-era custom resources is on NanoVDB: injectable CUDA memory resources — design & roadmap #2232.Design decisions (rationale on #2232)
noInittag is mandatory, matchingcuda::buffer(whose count constructor requirescuda::no_init) and the RMMdevice_uvectorlesson: implicit fill of freshly allocated memory is a hidden cost the dominant allocate-then-overwrite pattern wastes. Enforced byis_constructiblestatic_asserts.RBufferhas no stream API at all (member, constructor parameter,stream(),setStream— all absent, verified by detection-idiom static_asserts): a synchronous allocation is immediately valid on every stream, so no parameter exists to ignore.using Buffer = ::cuda::buffer<T>remains available at CUDA ≥ 13.2), member names are structural and keep CCCL's spelling.resize(overflow or allocation error) leaves the buffer fully untouched, including its retained stream (tested).Testing
New
unittest/TestBuffer.cu(TestBuffersuite, 20 tests; ctestnanovdb_cuda_buffer_unit_test): trait satisfaction for both tiers and the refinement; exact allocation sizes through a counting resource; stream retention,setStreamredirection, and resize stream ordering via a stream-recording resource; move/copy semantics without double-free; byte-size overflow throws; pinned page-lock; graph capture;BufferViewsemantics incl. the zero-copyGridHandleround-trip. Full suites green: CPU 153/153,nanovdb_test_cuda53/53, memory-resource 8/8; compute-sanitizer memcheck clean (sm_89, CUDA 12.6).Non-CUDA builds
No impact on
NANOVDB_USE_CUDA=OFF: new headers live undernanovdb/cuda/, and the test target is registered insideif(NANOVDB_USE_CUDA).🤖 Generated with Claude Code