diff --git a/docs/changelog/StorageBase-StealArray-returns-delete-function.md b/docs/changelog/StorageBase-StealArray-returns-delete-function.md new file mode 100644 index 0000000000000000000000000000000000000000..fa08f5f990714d57955ac70e3bd3706e5725329c --- /dev/null +++ b/docs/changelog/StorageBase-StealArray-returns-delete-function.md @@ -0,0 +1,24 @@ +## `StorageBasic` StealArray() now provides delete function to new owner + +Memory that is stolen from VTK-m has to be freed correctly. This is required +as the memory could have been allocated with `new`, `malloc` or even `cudaMallocManaged`. + +Previously it was very easy to transfer ownership of memory out of VTK-m and +either fail to capture the free function, or ask for it after the transfer +operation which would return a nullptr. Now stealing an array also +provides the free function reducing one source of memory leaks. + +To properly steal memory from VTK-m you do the following: +```cpp + vtkm::cont::ArrayHandle arrayHandle; + + ... + + auto* stolen = arrayHandle.StealArray(); + T* ptr = stolen.first; + auto free_function = stolen.second; + + ... + + free_function(ptr); +``` diff --git a/vtkm/cont/StorageBasic.h b/vtkm/cont/StorageBasic.h index 676dd270ce19c5189578418b69bfff3e0c7aa62b..86287d242bfca33a1372259d704b08580a1b7c2b 100644 --- a/vtkm/cont/StorageBasic.h +++ b/vtkm/cont/StorageBasic.h @@ -11,6 +11,7 @@ #define vtk_m_cont_StorageBasic_h #include +#include #include #include #include @@ -116,14 +117,6 @@ public: /// instances that have been stolen (\c StealArray) VTKM_CONT bool WillDeallocate() const { return this->DeleteFunction != nullptr; } - /// \brief Return the free function that will be used to free this memory. - /// - /// Get the function that VTK-m will call to deallocate the memory. This - /// is useful when stealing memory from VTK-m so that you call the correct - /// free/delete function when releasing the memory - using DeleteFunctionSignature = void (*)(void*); - DeleteFunctionSignature GetDeleteFunction() const { return this->DeleteFunction; } - /// \brief Change the Change the pointer that this class is using. Should only be used /// by ExecutionArrayInterface sublcasses. /// Note: This will release any previous allocated memory! @@ -197,17 +190,17 @@ public: VTKM_CONT const ValueType* GetArray() const; - /// \brief Take the reference away from this object. + /// \brief Transfer ownership of the underlying away from this object. /// - /// This method returns the pointer to the array held by this array. It then - /// clears the internal ownership flags, thereby ensuring that the - /// Storage will never deallocate the array or be able to reallocate it. This - /// is helpful for taking a reference for an array created internally by + /// This method returns the pointer and free function to the array held + /// by this array. It then clears the internal ownership flags, thereby ensuring + /// that the Storage will never deallocate the array or be able to reallocate it. + /// This is helpful for taking a reference for an array created internally by /// VTK-m and not having to keep a VTK-m object around. Obviously the caller - /// becomes responsible for destroying the memory, and should before hand - /// grab the correct DeleteFunction by calling \c GetDeleteFunction + /// becomes responsible for destroying the memory, and should use the provided + /// delete function. /// - VTKM_CONT ValueType* StealArray(); + VTKM_CONT vtkm::Pair StealArray(); }; } // namespace internal diff --git a/vtkm/cont/StorageBasic.hxx b/vtkm/cont/StorageBasic.hxx index 73f141003985daf0483d0f08e9e0cd2bebe09093..7655d690b4e46131beefaae5641ac87f08d4af22 100644 --- a/vtkm/cont/StorageBasic.hxx +++ b/vtkm/cont/StorageBasic.hxx @@ -103,10 +103,11 @@ const T* Storage::GetArray() const } template -T* Storage::StealArray() +vtkm::Pair Storage::StealArray() { + vtkm::Pair result(static_cast(this->Array), this->DeleteFunction); this->DeleteFunction = nullptr; - return static_cast(this->Array); + return result; } } // namespace internal diff --git a/vtkm/cont/testing/UnitTestStorageBasic.cxx b/vtkm/cont/testing/UnitTestStorageBasic.cxx index b84b3132fb63ef442658ca5aaf3428db35f5e13d..c1581586f38f19b423537abacf73089b24da6f01 100644 --- a/vtkm/cont/testing/UnitTestStorageBasic.cxx +++ b/vtkm/cont/testing/UnitTestStorageBasic.cxx @@ -74,7 +74,8 @@ struct TemplatedTests // This call steals the array and prevents deallocation. VTKM_TEST_ASSERT(stealMyArray.WillDeallocate() == true, "Array to be stolen needs to be owned by VTK-m"); - stolenArray = stealMyArray.StealArray(); + auto stolen = stealMyArray.StealArray(); + stolenArray = stolen.first; VTKM_TEST_ASSERT(stealMyArray.WillDeallocate() == false, "Stolen array should not be owned by VTK-m"); diff --git a/vtkm/rendering/MapperGL.cxx b/vtkm/rendering/MapperGL.cxx index 4941dc8ad63f6dfc0427b10157cd5ad7e79f1014..57828bdb116be86371249659381ee389581d58e8 100644 --- a/vtkm/rendering/MapperGL.cxx +++ b/vtkm/rendering/MapperGL.cxx @@ -291,8 +291,8 @@ VTKM_CONT void RenderTriangles(MapperGL& mapper, vtkm::Id vtx_cnt = out_vertices.GetNumberOfValues(); - vtkm::Float32* v_ptr = out_vertices.GetStorage().StealArray(); - vtkm::Float32* c_ptr = out_color.GetStorage().StealArray(); + vtkm::Float32* v_ptr = out_vertices.GetStorage().GetArray(); + vtkm::Float32* c_ptr = out_color.GetStorage().GetArray(); vtkm::Id floatSz = static_cast(sizeof(vtkm::Float32)); GLsizeiptr sz = static_cast(vtx_cnt * floatSz);