From a2bda0610d22afce8262bfeee2cc18c8b6b6ff48 Mon Sep 17 00:00:00 2001 From: Kyle Lutz Date: Mon, 13 May 2013 22:10:03 -0400 Subject: [PATCH] Fix memory issues with device_ptr and allocator This fixes a few memory handling issues between device_ptr, buffer_iterator, buffer_value, allocator, and malloc/free. Previously, memory buffers that were allocated by allocator and malloc were being retained (via clRetainMemObject() in buffer's constructor) by device_ptr, buffer_iterator and buffer_value. Now, false is passed for the retain parameter to buffer's constructor so that the buffer's reference count is not incremented. Furthermore, the classes now set the buffer to null before being destructed so that they will not decrement its reference count (which normally occurs buffer's destructor). The main effect of this change is that objects which refer to a memory buffer but do not own it (e.g. device_ptr, buffer_iterator) will not modify the reference count for the buffer. This fixes a number of memory leaks which occured in longer running programs. --- include/boost/compute/container/allocator.hpp | 6 ++- include/boost/compute/detail/buffer_value.hpp | 37 +++++++++++-------- include/boost/compute/detail/meta_kernel.hpp | 8 ++-- include/boost/compute/device_ptr.hpp | 37 +++++++++---------- .../compute/iterator/buffer_iterator.hpp | 26 +++++++------ include/boost/compute/malloc.hpp | 11 ++---- 6 files changed, 64 insertions(+), 61 deletions(-) diff --git a/include/boost/compute/container/allocator.hpp b/include/boost/compute/container/allocator.hpp index a26193cc..4e8fb992 100644 --- a/include/boost/compute/container/allocator.hpp +++ b/include/boost/compute/container/allocator.hpp @@ -56,7 +56,9 @@ public: pointer allocate(size_type n) { - return device_ptr(buffer(m_context, n * sizeof(T), m_mem_flags)); + buffer buf(m_context, n * sizeof(T), m_mem_flags); + clRetainMemObject(buf.get()); + return device_ptr(buf); } void deallocate(pointer p, size_type n) @@ -65,7 +67,7 @@ public: (void) n; - delete p.get_buffer_ptr(); + clReleaseMemObject(p.get_buffer().get()); } size_type max_size() const diff --git a/include/boost/compute/detail/buffer_value.hpp b/include/boost/compute/detail/buffer_value.hpp index 60a111e7..51fbfee5 100644 --- a/include/boost/compute/detail/buffer_value.hpp +++ b/include/boost/compute/detail/buffer_value.hpp @@ -27,37 +27,42 @@ public: typedef T value_type; buffer_value() - : m_buffer(0) { } buffer_value(const value_type &value) - : m_buffer(0), - m_value(value) + : m_value(value) { } // creates a reference for the value in buffer at index (in bytes). buffer_value(const buffer &buffer, size_t index) - : m_buffer(&buffer), + : m_buffer(buffer.get(), false), m_index(index) { } buffer_value(const buffer_value &other) - : m_buffer(other.m_buffer), + : m_buffer(other.m_buffer.get(), false), m_index(other.m_index) { } + ~buffer_value() + { + // set buffer to null so that its reference count will + // not be decremented when its destructor is called + m_buffer.get() = 0; + } + operator value_type() const { - if(m_buffer){ - const context &context = m_buffer->get_context(); + if(m_buffer.get()){ + const context &context = m_buffer.get_context(); const device &device = context.get_device(); command_queue queue(context, device); - return detail::read_single_value(*m_buffer, m_index / sizeof(T), queue); + return detail::read_single_value(m_buffer, m_index / sizeof(T), queue); } else { return m_value; @@ -96,11 +101,11 @@ public: bool operator==(const buffer_value &other) const { - if(m_buffer != other.m_buffer){ + if(m_buffer.get() != other.m_buffer.get()){ return false; } - if(m_buffer){ + if(m_buffer.get()){ return m_index == other.m_index; } else { @@ -115,11 +120,11 @@ public: buffer_value& operator=(const T &value) { - if(m_buffer){ - const context &context = m_buffer->get_context(); + if(m_buffer.get()){ + const context &context = m_buffer.get_context(); command_queue queue(context, context.get_device()); - detail::write_single_value(value, *m_buffer, m_index / sizeof(T), queue); + detail::write_single_value(value, m_buffer, m_index / sizeof(T), queue); return *this; } @@ -136,12 +141,12 @@ public: device_ptr operator&() const { - return device_ptr(*m_buffer, m_index); + return device_ptr(m_buffer, m_index); } buffer_value& operator++() { - if(m_buffer){ + if(m_buffer.get()){ T value = T(*this); value++; *this = value; @@ -161,7 +166,7 @@ public: } private: - const buffer *m_buffer; + const buffer m_buffer; size_t m_index; value_type m_value; }; diff --git a/include/boost/compute/detail/meta_kernel.hpp b/include/boost/compute/detail/meta_kernel.hpp index 8ecbc8d2..ce53f54e 100644 --- a/include/boost/compute/detail/meta_kernel.hpp +++ b/include/boost/compute/detail/meta_kernel.hpp @@ -185,14 +185,14 @@ struct meta_kernel_buffer_info const std::string &id, const std::string &addr_space, size_t i) - : m_buffer(&buffer), + : m_mem(buffer.get()), identifier(id), address_space(addr_space), index(i) { } - const buffer *m_buffer; + cl_mem m_mem; std::string identifier; std::string address_space; size_t index; @@ -336,7 +336,7 @@ public: for(size_t i = 0; i < m_stored_buffers.size(); i++){ const detail::meta_kernel_buffer_info &bi = m_stored_buffers[i]; - kernel.set_arg(bi.index, *bi.m_buffer); + kernel.set_arg(bi.index, bi.m_mem); } return kernel; @@ -574,7 +574,7 @@ public: for(size_t i = 0; i < m_stored_buffers.size(); i++){ const detail::meta_kernel_buffer_info &bi = m_stored_buffers[i]; - if(bi.m_buffer->get() == buffer.get() && + if(bi.m_mem == buffer.get() && bi.address_space == address_space){ return bi.identifier; } diff --git a/include/boost/compute/device_ptr.hpp b/include/boost/compute/device_ptr.hpp index d44005c0..9ee7dcb9 100644 --- a/include/boost/compute/device_ptr.hpp +++ b/include/boost/compute/device_ptr.hpp @@ -44,6 +44,8 @@ struct device_ptr_index_expr BOOST_STATIC_ASSERT_MSG(boost::is_integral::value, "Index expression must be integral"); + BOOST_ASSERT(m_buffer.get()); + const context &context = m_buffer.get_context(); const device &device = context.get_device(); command_queue queue(context, device); @@ -70,19 +72,18 @@ public: typedef T& reference; device_ptr() - : m_buffer(0), - m_index(0) + : m_index(0) { } device_ptr(const buffer &buffer, size_t index = 0) - : m_buffer(new ::boost::compute::buffer(buffer)), + : m_buffer(buffer.get(), false), m_index(index) { } device_ptr(const device_ptr &other) - : m_buffer(other.m_buffer), + : m_buffer(other.m_buffer.get(), false), m_index(other.m_index) { } @@ -90,7 +91,7 @@ public: device_ptr& operator=(const device_ptr &other) { if(this != &other){ - m_buffer = other.m_buffer; + m_buffer.get() = other.m_buffer.get(); m_index = other.m_index; } @@ -99,6 +100,9 @@ public: ~device_ptr() { + // set buffer to null so that its reference count will + // not be decremented when its destructor is called + m_buffer.get() = 0; } size_type get_index() const @@ -107,13 +111,6 @@ public: } const buffer& get_buffer() const - { - BOOST_ASSERT(m_buffer != 0); - - return *m_buffer; - } - - const buffer* get_buffer_ptr() const { return m_buffer; } @@ -121,17 +118,17 @@ public: template device_ptr cast() const { - return device_ptr(*m_buffer, m_index); + return device_ptr(m_buffer, m_index); } device_ptr operator+(difference_type n) const { - return device_ptr(*m_buffer, m_index + n); + return device_ptr(m_buffer, m_index + n); } device_ptr operator+(const device_ptr &other) const { - return device_ptr(*m_buffer, m_index + other.m_index); + return device_ptr(m_buffer, m_index + other.m_index); } device_ptr& operator+=(difference_type n) @@ -153,7 +150,8 @@ public: bool operator==(const device_ptr &other) const { - return *m_buffer == *other.m_buffer && m_index == other.m_index; + return m_buffer.get() == other.m_buffer.get() && + m_index == other.m_index; } bool operator!=(const device_ptr &other) const @@ -165,16 +163,15 @@ public: detail::device_ptr_index_expr operator[](const Expr &expr) const { - BOOST_ASSERT(m_buffer); - BOOST_ASSERT(m_buffer->get()); + BOOST_ASSERT(m_buffer.get()); - return detail::device_ptr_index_expr(*m_buffer, + return detail::device_ptr_index_expr(m_buffer, uint_(m_index), expr); } private: - const buffer *m_buffer; + const buffer m_buffer; size_t m_index; }; diff --git a/include/boost/compute/iterator/buffer_iterator.hpp b/include/boost/compute/iterator/buffer_iterator.hpp index 187ea903..e3ee5bc8 100644 --- a/include/boost/compute/iterator/buffer_iterator.hpp +++ b/include/boost/compute/iterator/buffer_iterator.hpp @@ -105,19 +105,18 @@ public: typedef typename super_type::difference_type difference_type; buffer_iterator() - : m_buffer(0), - m_index(0) + : m_index(0) { } buffer_iterator(const buffer &buffer, size_t index) - : m_buffer(&buffer), + : m_buffer(buffer.get(), false), m_index(index) { } buffer_iterator(const buffer_iterator &other) - : m_buffer(other.m_buffer), + : m_buffer(other.m_buffer.get(), false), m_index(other.m_index) { } @@ -125,7 +124,7 @@ public: buffer_iterator& operator=(const buffer_iterator &other) { if(this != &other){ - m_buffer = other.m_buffer; + m_buffer.get() = other.m_buffer.get(); m_index = other.m_index; } @@ -134,11 +133,14 @@ public: ~buffer_iterator() { + // set buffer to null so that its reference count will + // not be decremented when its destructor is called + m_buffer.get() = 0; } const buffer& get_buffer() const { - return *m_buffer; + return m_buffer; } size_t get_index() const @@ -150,10 +152,9 @@ public: detail::buffer_iterator_index_expr operator[](const Expr &expr) const { - BOOST_ASSERT(m_buffer); - BOOST_ASSERT(m_buffer->get()); + BOOST_ASSERT(m_buffer.get()); - return detail::buffer_iterator_index_expr(*m_buffer, + return detail::buffer_iterator_index_expr(m_buffer, m_index, "__global", expr); @@ -164,12 +165,13 @@ private: reference dereference() const { - return detail::buffer_value(*m_buffer, m_index * sizeof(T)); + return detail::buffer_value(m_buffer, m_index * sizeof(T)); } bool equal(const buffer_iterator &other) const { - return m_buffer == other.m_buffer && m_index == other.m_index; + return m_buffer.get() == other.m_buffer.get() && + m_index == other.m_index; } void increment() @@ -193,7 +195,7 @@ private: } private: - const buffer *m_buffer; + const buffer m_buffer; size_t m_index; }; diff --git a/include/boost/compute/malloc.hpp b/include/boost/compute/malloc.hpp index f18e6f92..b72ac891 100644 --- a/include/boost/compute/malloc.hpp +++ b/include/boost/compute/malloc.hpp @@ -23,7 +23,9 @@ template inline device_ptr malloc(std::size_t size, const context &context = system::default_context()) { - return device_ptr(buffer(context, size * sizeof(T))); + buffer buf(context, size * sizeof(T)); + clRetainMemObject(buf.get()); + return device_ptr(buf); } inline device_ptr @@ -35,12 +37,7 @@ malloc(std::size_t size, const context &context = system::default_context()) template inline void free(device_ptr &ptr) { - const buffer *buffer = ptr.get_buffer_ptr(); - if(buffer){ - delete buffer; - } - - ptr = device_ptr(); + clReleaseMemObject(ptr.get_buffer().get()); } } // end compute namespace