NanoVDB: expose ResourceT on the five TopologyBuilder consumers (CUDA) - #2273
Open
swahtz wants to merge 12 commits into
Open
NanoVDB: expose ResourceT on the five TopologyBuilder consumers (CUDA)#2273swahtz wants to merge 12 commits into
swahtz wants to merge 12 commits into
Conversation
Name the free operation destroy, as cuda::buffer does, and add the stream-taking overload it also provides. clear stays but only as a transitional delegate, marked as such: it exists because GridHandle::reset still calls it, and goes away with the legacy dual buffers that cuda::Buffer replaces. Rename setStream to set_stream. Member names cannot be aliased, so matching the standard spelling is the whole reason the resource concept kept allocate_async; the same argument applies here. Note in passing that ours deliberately does not synchronize, matching cuda::buffer's set_stream_unsynchronized rather than its set_stream, whose own documentation and implementation disagree (NVIDIA/cccl#10649). Add swap. The generic std::swap already does the right thing through the move operations, but both std::vector and rmm::device_buffer provide one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
Eleven of the builder's buffers are device-only -- nothing reads them on the host -- yet they were cuda::DeviceBuffer, whose host pointer and per-device array they never use. Move them to the single-space cuda::Buffer and give the builder a resource parameter, so its scratch is injectable like PointsToGrid's already is, and freed by scope rather than by hand. mProcessedRoot and mData are read on the host and stay dual. This is not a speedup: DeviceBuffer::init allocates host or device memory but never both, and these buffers always passed a real device id, so no cudaMallocHost was on this path to begin with. Measured dilate on 1k/20k/ 200k points, and the difference is within run-to-run noise. Hoist the Data struct out of the class. It does not depend on the resource, and leaving it nested would give every ResourceT its own incompatible type for the device functors to name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
A stream-ordered resource must also model the synchronous concept, which means writing four methods where two would do. The synchronous pair is not a bare delegate -- memory from allocate must be usable on any stream when it returns, so the null-stream allocation has to be synchronized first -- and omitting that yields memory which satisfies the concept but is not actually synchronous. Put it in one place rather than leaving each author to rediscover it. The two resources in TestMemoryResource are the first users, and were already wrong in exactly that way: they provide only the async pair, so they never modelled is_async_resource. TempPool duck-typed and never checked, so nothing caught it. MeshToGrid was the last builder allocating from a hard-wired DeviceResource, through TempDevicePool. Give it a ResourceT parameter and thread it into both its TopologyBuilder and its pool. As with Data in the builder, BoxTrianglePair is hoisted out of the class: it does not depend on the resource, and leaving it nested would give every ResourceT its own incompatible type for the device functors to name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
…uffer Buffer holds its resource by value, matching cuda::buffer -- whose model this completes: in CCCL the ownership semantics are selected by what is placed in the by-value slot, an owning any_resource or a borrowing resource_ref. We adopted the slot without the borrowing type, so a container like TempPool, whose contract is a non-owning pointer to a possibly stateful resource, had no way to hold a Buffer without copying that resource and stranding its state. ResourceRef is the missing piece: a non-owning reference that is itself a resource, so copying the ref shares the underlying instance. Its async methods exist only when R models AsyncResource, so a ref over a synchronous resource does not misreport its tier, and two refs compare equal exactly when they reference the same resource. TempPool now keeps its bytes in a Buffer<std::byte, ResourceRef<R>>: same resource contract, same stream retention, same discard-on-growth reallocation, but the block is freed by ownership rather than by hand. The TempPool unit tests, which assert traffic against the caller's own resource instance, pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
Each release site sits in a function that receives the stream, so pass it to destroy rather than relying on the retained stream matching -- they are the same on every current path, but the explicit form does not depend on that staying true. Also drop the cudaGetDevice calls whose result the Buffer conversion left unused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
Signed-off-by: Mark Harris <mharris@nvidia.com>
The scratch buffers held their resource by value, so each of the eight carried its own copy -- fine for the stateless default, wrong for a stateful resource, whose accounting would be split across copies while the caller's instance saw nothing. Borrow through ResourceRef instead, the same reconciliation TempPool uses. Assert the stream-ordered requirement directly in TopologyBuilder and MeshToGrid so a synchronous-only resource fails with a diagnostic that names the builder, not just the pool inside it. Note SyncFromAsync's synchronize cost on its allocate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
The byte scratch is reinterpreted as word-sized types, which is valid for every resource whose DEFAULT_ALIGNMENT is at least word alignment -- all CUDA allocation paths give 256 -- but nothing said so. Assert it, so a custom resource with a weaker guarantee fails at compile time instead of misaligning on the device. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
cuda::buffer's set_stream is deliberately non-synchronizing -- its documented synchronization is a stale note left behind when the synchronization was removed -- so our non-synchronizing set_stream matches it in both name and contract, and the comment claiming a divergence was wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Mark Harris <mharris@nvidia.com>
Signed-off-by: Mark Harris <mharris@nvidia.com>
DilateGrid, MergeGrids, PruneGrid, RefineGrid and CoarsenGrid each hold a TopologyBuilder that was constructed as mBuilder(stream), binding the per-type default DeviceResource with no injection path. This extends the seam pattern established for PointsToGrid (AcademySoftwareFoundation#2268) and MeshToGrid (AcademySoftwareFoundation#2269) to these five operators so their stream-ordered device scratch can route through an injected memory resource -- the last piece downstream (fvdb-core) needs before it can delete its forked headers (openvdb AcademySoftwareFoundation#2232, B5). Each class gains a `ResourceT` template parameter (defaulted to nanovdb::cuda::DeviceResource) with an is_async_resource guard assert, its TopologyBuilder member becomes TopologyBuilder<BuildT, ResourceT>, and its constructor(s) gain a trailing defaulted `ResourceT& resource` argument forwarded to the builder. All out-of-class member definitions are updated to the two-parameter form. The change is additive-in-behavior: ResourceT defaults everywhere, so every existing caller compiles unchanged with identical allocation behavior. The deliberately-untouched dual-space buffers (each operator's mProcessedRoot and the builder's mData DeviceBuffer) remain on DeviceBuffer and are Step 3's dual-space problem. Adds one injected-resource test per operator to TestMemoryResource.cu, mirroring PointsToGrid_RunsOnSynchronousResource: build a small ValueOnIndex source grid, run the op with a CountingResource, and assert allocs > 0 && allocs == deallocs. Local GPU verification (RTX PRO 6000 Blackwell, CUDA 13.0): nanovdb_test_cuda_memory_resource: 15/15 (10 existing + 5 new) nanovdb_test_cuda_buffer: 27/27 nanovdb_test_cuda: 52/53 (UnifiedBuffer_IO fails on baseline too -- missing data/3_spheres.nvdb) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
swahtz
requested review from
Idclip,
apradhana,
danrbailey,
jmlait,
kmuseth and
richhones
as code owners
August 7, 2026 04:27
3 tasks
harrism
approved these changes
Aug 11, 2026
harrism
left a comment
harrism
left a comment
Contributor
There was a problem hiding this comment.
Looks good. Thanks for extending Resources to the topologyBuilder consumers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2231/#2251, part of #2232. This is B5: the five
tools::cuda::TopologyBuilderconsumers get the same injectable-resource seam that #2268 gave the builder and #2269 gaveMeshToGrid.What / why
#2268 gave
tools::cuda::TopologyBuilderaResourceTtemplate parameter so its device scratch + cubTempPoolallocate through an injected resource. But the five classes that instantiate it —DilateGrid,MergeGrids,PruneGrid,RefineGrid,CoarsenGrid(all innanovdb/tools/cuda/) — were left as unaffected callers: each heldTopologyBuilder<BuildT> mBuilder;constructed asmBuilder(stream), bindingdefault_resource<DeviceResource>()with no injection path. Downstream (fvdb-core) needs to route these ops' scratch through PyTorch'sc10::cuda::CUDACachingAllocator; without this seam it cannot delete its forked headers.The pattern (established by
MeshToGridin #2269)tools/cuda/MeshToGrid.cuhis the reference. Each of the five headers receives the same conversion:typename ResourceT = nanovdb::cuda::DeviceResource, with anis_async_resourceguardstatic_assertfirst in the class body.TopologyBuilder<BuildT, ResourceT> mBuilder;.ResourceT& resource = nanovdb::cuda::default_resource<ResourceT>(), forwarded asmBuilder(stream, resource). Trailing position keeps every existing call source-compatible;MergeGridsgets the argument on both its N-ary and its binary-convenience constructor, and the delegating one forwards it.template<typename BuildT, typename ResourceT>/ClassName<BuildT, ResourceT>::), including the dual-templategetHandleand the} // ClassName<...>::methodcomment trailers.No nested-type hoisting is needed:
TopologyBuilder::Datawas already hoisted toTopologyBuilderDatain #2268, and none of the five classes declares its own nested struct.The change is additive-in-behavior:
ResourceTdefaults everywhere, so every existing caller compiles unchanged with identical allocation behavior.Deliberately untouched — Step 3 residue
Each operator's
mBuilder.mProcessedRootandTopologyBuilder's internalmDataare stillnanovdb::cuda::DeviceBuffer(host+device dual-space) and are left out of scope here — they are Step 3's dual-space problem. The grid handle's output buffer likewise goes throughBufferT::create. None of these is observed by the injected resource, which is expected and is called out in the new tests' comments.Testing
Adds one injected-resource test per operator to
TestMemoryResource.cu(DilateGrid_InjectedResourceSeam, etc.), mirroring the existingPointsToGridseam tests: build a smallValueOnIndexsource grid, run the op with aCountingResourcepassed as the new trailing constructor argument, synchronize, and assertallocs > 0 && allocs == deallocs.MergeGridsmerges two disjoint grids;PruneGridsupplies a single all-onMask<3>sidecar over a one-leaf grid;CoarsenGridspreads voxels over a 2×2×2 leaf block so coarsening is non-degenerate. A pendingchanges note covers the five new seams.Verified locally on an RTX PRO 6000 Blackwell, CUDA 13.0:
nanovdb_test_cuda_memory_resource— 14/14 (9 existing + 5 new)nanovdb_test_cuda_buffer— 27/27, unchangednanovdb_test_cuda— 52/53; the failure isUnifiedBuffer_IO, a missing test-data file (data/3_spheres.nvdb) in an unrelated component, which fails identically on an unmodified baseline. The full CUDA suite also exercises all five ops through their defaulted parameter (dilate/merge/prune/refine/coarsen tests), confirming zero impact on default callers.Per #2264 the CUDA tests are excluded from CI, so CI will build but not run these.