From 765157004f38cf0e2978aa05c7e053ddf243bda0 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Mon, 11 Jul 2016 23:40:38 +0900 Subject: [PATCH 1/7] Change InternalMemoryPool::Free() to return Status::Invalid when there is insufficient memory. --- cpp/src/arrow/util/memory-pool-test.cc | 10 +++++++++- cpp/src/arrow/util/memory-pool.cc | 17 ++++++++++++----- cpp/src/arrow/util/memory-pool.h | 4 ++-- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/cpp/src/arrow/util/memory-pool-test.cc b/cpp/src/arrow/util/memory-pool-test.cc index 8e7dfd60baa62..c7d45d58fa5e6 100644 --- a/cpp/src/arrow/util/memory-pool-test.cc +++ b/cpp/src/arrow/util/memory-pool-test.cc @@ -34,7 +34,7 @@ TEST(DefaultMemoryPool, MemoryTracking) { EXPECT_EQ(static_cast(0), reinterpret_cast(data) % 64); ASSERT_EQ(100, pool->bytes_allocated()); - pool->Free(data, 100); + ASSERT_OK(pool->Free(data, 100)); ASSERT_EQ(0, pool->bytes_allocated()); } @@ -46,4 +46,12 @@ TEST(DefaultMemoryPool, OOM) { ASSERT_RAISES(OutOfMemory, pool->Allocate(to_alloc, &data)); } +TEST(DefaultMemoryPool, FreeInsufficientMemory) { + MemoryPool* pool = default_memory_pool(); + + uint8_t* data; + ASSERT_OK(pool->Allocate(128, &data)); + ASSERT_RAISES(Invalid, pool->Free(data, 256)); +} + } // namespace arrow diff --git a/cpp/src/arrow/util/memory-pool.cc b/cpp/src/arrow/util/memory-pool.cc index 0a58e5aa21f72..af68265303eb5 100644 --- a/cpp/src/arrow/util/memory-pool.cc +++ b/cpp/src/arrow/util/memory-pool.cc @@ -57,13 +57,13 @@ class InternalMemoryPool : public MemoryPool { Status Allocate(int64_t size, uint8_t** out) override; - void Free(uint8_t* buffer, int64_t size) override; + Status Free(uint8_t* buffer, int64_t size) override; - int64_t bytes_allocated() const override; + uint64_t bytes_allocated() const override; private: mutable std::mutex pool_lock_; - int64_t bytes_allocated_; + uint64_t bytes_allocated_; }; Status InternalMemoryPool::Allocate(int64_t size, uint8_t** out) { @@ -74,15 +74,22 @@ Status InternalMemoryPool::Allocate(int64_t size, uint8_t** out) { return Status::OK(); } -int64_t InternalMemoryPool::bytes_allocated() const { +uint64_t InternalMemoryPool::bytes_allocated() const { std::lock_guard guard(pool_lock_); return bytes_allocated_; } -void InternalMemoryPool::Free(uint8_t* buffer, int64_t size) { +Status InternalMemoryPool::Free(uint8_t* buffer, int64_t size) { std::lock_guard guard(pool_lock_); + if (bytes_allocated_ < size) { + std::stringstream ss; + ss << "free of size " << size << " larger than allocated bytes " << bytes_allocated_ << " failed "; + return Status::Invalid(ss.str()); + } std::free(buffer); bytes_allocated_ -= size; + + return Status::OK(); } InternalMemoryPool::~InternalMemoryPool() {} diff --git a/cpp/src/arrow/util/memory-pool.h b/cpp/src/arrow/util/memory-pool.h index 4c1d699addd50..daffdcf25e043 100644 --- a/cpp/src/arrow/util/memory-pool.h +++ b/cpp/src/arrow/util/memory-pool.h @@ -31,9 +31,9 @@ class ARROW_EXPORT MemoryPool { virtual ~MemoryPool(); virtual Status Allocate(int64_t size, uint8_t** out) = 0; - virtual void Free(uint8_t* buffer, int64_t size) = 0; + virtual Status Free(uint8_t* buffer, int64_t size) = 0; - virtual int64_t bytes_allocated() const = 0; + virtual uint64_t bytes_allocated() const = 0; }; ARROW_EXPORT MemoryPool* default_memory_pool(); From e89a1f99ab365561ee9224dbf5a81c2715bd5236 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Tue, 12 Jul 2016 00:08:24 +0900 Subject: [PATCH 2/7] Change python implementation as well. --- python/pyarrow/includes/libarrow.pxd | 2 +- python/src/pyarrow/common.cc | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/python/pyarrow/includes/libarrow.pxd b/python/pyarrow/includes/libarrow.pxd index 91ce069df8f42..f34378f74b978 100644 --- a/python/pyarrow/includes/libarrow.pxd +++ b/python/pyarrow/includes/libarrow.pxd @@ -51,7 +51,7 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil: c_string ToString() cdef cppclass MemoryPool" arrow::MemoryPool": - int64_t bytes_allocated() + uint64_t bytes_allocated() cdef MemoryPool* default_memory_pool() diff --git a/python/src/pyarrow/common.cc b/python/src/pyarrow/common.cc index a2748f99b6733..d0ea6aa071e29 100644 --- a/python/src/pyarrow/common.cc +++ b/python/src/pyarrow/common.cc @@ -47,15 +47,22 @@ class PyArrowMemoryPool : public arrow::MemoryPool { return arrow::Status::OK(); } - int64_t bytes_allocated() const override { + uint64_t bytes_allocated() const override { std::lock_guard guard(pool_lock_); return bytes_allocated_; } - void Free(uint8_t* buffer, int64_t size) override { + arrow::Status Free(uint8_t* buffer, int64_t size) override { std::lock_guard guard(pool_lock_); + if (bytes_allocated_ < size) { + std::stringstream ss; + ss << "free of size " << size << " larger than allocated bytes " << bytes_allocated_ << " failed "; + return arrow::Status::Invalid(ss.str()); + } std::free(buffer); bytes_allocated_ -= size; + + return arrow::Status::OK(); } private: From b4159f00d4fb97ae6ee68528961beb20413c6659 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Tue, 12 Jul 2016 23:58:29 +0900 Subject: [PATCH 3/7] Reflect comments --- cpp/src/arrow/util/logging.h | 2 +- cpp/src/arrow/util/memory-pool-test.cc | 7 ++++--- cpp/src/arrow/util/memory-pool.cc | 19 +++++++------------ cpp/src/arrow/util/memory-pool.h | 4 ++-- python/pyarrow/includes/libarrow.pxd | 2 +- python/src/pyarrow/common.cc | 11 ++--------- 6 files changed, 17 insertions(+), 28 deletions(-) diff --git a/cpp/src/arrow/util/logging.h b/cpp/src/arrow/util/logging.h index fccc5e3085de5..54f67593bec5e 100644 --- a/cpp/src/arrow/util/logging.h +++ b/cpp/src/arrow/util/logging.h @@ -40,7 +40,7 @@ namespace arrow { #define ARROW_CHECK(condition) \ (condition) ? 0 : ::arrow::internal::FatalLog(ARROW_FATAL) \ - << __FILE__ << __LINE__ << "Check failed: " #condition " " + << __FILE__ << __LINE__ << " Check failed: " #condition " " #ifdef NDEBUG #define ARROW_DFATAL ARROW_WARNING diff --git a/cpp/src/arrow/util/memory-pool-test.cc b/cpp/src/arrow/util/memory-pool-test.cc index c7d45d58fa5e6..6d701d8173779 100644 --- a/cpp/src/arrow/util/memory-pool-test.cc +++ b/cpp/src/arrow/util/memory-pool-test.cc @@ -34,7 +34,7 @@ TEST(DefaultMemoryPool, MemoryTracking) { EXPECT_EQ(static_cast(0), reinterpret_cast(data) % 64); ASSERT_EQ(100, pool->bytes_allocated()); - ASSERT_OK(pool->Free(data, 100)); + pool->Free(data, 100); ASSERT_EQ(0, pool->bytes_allocated()); } @@ -46,12 +46,13 @@ TEST(DefaultMemoryPool, OOM) { ASSERT_RAISES(OutOfMemory, pool->Allocate(to_alloc, &data)); } -TEST(DefaultMemoryPool, FreeInsufficientMemory) { +TEST(DefaultMemoryPool, FreeLargeMemory) { MemoryPool* pool = default_memory_pool(); uint8_t* data; ASSERT_OK(pool->Allocate(128, &data)); - ASSERT_RAISES(Invalid, pool->Free(data, 256)); + ASSERT_DEATH(pool->Free(data, 256), + ".*Check failed: \\(bytes_allocated_\\) >= \\(size\\)"); } } // namespace arrow diff --git a/cpp/src/arrow/util/memory-pool.cc b/cpp/src/arrow/util/memory-pool.cc index af68265303eb5..fed149bc3598c 100644 --- a/cpp/src/arrow/util/memory-pool.cc +++ b/cpp/src/arrow/util/memory-pool.cc @@ -23,6 +23,7 @@ #include #include "arrow/util/status.h" +#include "arrow/util/logging.h" namespace arrow { @@ -57,13 +58,13 @@ class InternalMemoryPool : public MemoryPool { Status Allocate(int64_t size, uint8_t** out) override; - Status Free(uint8_t* buffer, int64_t size) override; + void Free(uint8_t* buffer, int64_t size) override; - uint64_t bytes_allocated() const override; + int64_t bytes_allocated() const override; private: mutable std::mutex pool_lock_; - uint64_t bytes_allocated_; + int64_t bytes_allocated_; }; Status InternalMemoryPool::Allocate(int64_t size, uint8_t** out) { @@ -74,22 +75,16 @@ Status InternalMemoryPool::Allocate(int64_t size, uint8_t** out) { return Status::OK(); } -uint64_t InternalMemoryPool::bytes_allocated() const { +int64_t InternalMemoryPool::bytes_allocated() const { std::lock_guard guard(pool_lock_); return bytes_allocated_; } -Status InternalMemoryPool::Free(uint8_t* buffer, int64_t size) { +void InternalMemoryPool::Free(uint8_t* buffer, int64_t size) { std::lock_guard guard(pool_lock_); - if (bytes_allocated_ < size) { - std::stringstream ss; - ss << "free of size " << size << " larger than allocated bytes " << bytes_allocated_ << " failed "; - return Status::Invalid(ss.str()); - } + DCHECK_GE(bytes_allocated_, size); std::free(buffer); bytes_allocated_ -= size; - - return Status::OK(); } InternalMemoryPool::~InternalMemoryPool() {} diff --git a/cpp/src/arrow/util/memory-pool.h b/cpp/src/arrow/util/memory-pool.h index daffdcf25e043..4c1d699addd50 100644 --- a/cpp/src/arrow/util/memory-pool.h +++ b/cpp/src/arrow/util/memory-pool.h @@ -31,9 +31,9 @@ class ARROW_EXPORT MemoryPool { virtual ~MemoryPool(); virtual Status Allocate(int64_t size, uint8_t** out) = 0; - virtual Status Free(uint8_t* buffer, int64_t size) = 0; + virtual void Free(uint8_t* buffer, int64_t size) = 0; - virtual uint64_t bytes_allocated() const = 0; + virtual int64_t bytes_allocated() const = 0; }; ARROW_EXPORT MemoryPool* default_memory_pool(); diff --git a/python/pyarrow/includes/libarrow.pxd b/python/pyarrow/includes/libarrow.pxd index f34378f74b978..91ce069df8f42 100644 --- a/python/pyarrow/includes/libarrow.pxd +++ b/python/pyarrow/includes/libarrow.pxd @@ -51,7 +51,7 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil: c_string ToString() cdef cppclass MemoryPool" arrow::MemoryPool": - uint64_t bytes_allocated() + int64_t bytes_allocated() cdef MemoryPool* default_memory_pool() diff --git a/python/src/pyarrow/common.cc b/python/src/pyarrow/common.cc index d0ea6aa071e29..a2748f99b6733 100644 --- a/python/src/pyarrow/common.cc +++ b/python/src/pyarrow/common.cc @@ -47,22 +47,15 @@ class PyArrowMemoryPool : public arrow::MemoryPool { return arrow::Status::OK(); } - uint64_t bytes_allocated() const override { + int64_t bytes_allocated() const override { std::lock_guard guard(pool_lock_); return bytes_allocated_; } - arrow::Status Free(uint8_t* buffer, int64_t size) override { + void Free(uint8_t* buffer, int64_t size) override { std::lock_guard guard(pool_lock_); - if (bytes_allocated_ < size) { - std::stringstream ss; - ss << "free of size " << size << " larger than allocated bytes " << bytes_allocated_ << " failed "; - return arrow::Status::Invalid(ss.str()); - } std::free(buffer); bytes_allocated_ -= size; - - return arrow::Status::OK(); } private: From b1af59b9d39c849c18509ccfcbc97173d3172143 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Wed, 13 Jul 2016 00:35:14 +0900 Subject: [PATCH 4/7] Change to ASSERT_EXIT --- cpp/src/arrow/util/memory-pool-test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/util/memory-pool-test.cc b/cpp/src/arrow/util/memory-pool-test.cc index 6d701d8173779..bdc66cde1cca4 100644 --- a/cpp/src/arrow/util/memory-pool-test.cc +++ b/cpp/src/arrow/util/memory-pool-test.cc @@ -51,7 +51,7 @@ TEST(DefaultMemoryPool, FreeLargeMemory) { uint8_t* data; ASSERT_OK(pool->Allocate(128, &data)); - ASSERT_DEATH(pool->Free(data, 256), + ASSERT_EXIT(pool->Free(data, 256), ::testing::ExitedWithCode(1), ".*Check failed: \\(bytes_allocated_\\) >= \\(size\\)"); } From 0077a700963d8e28935627246c081e487933121f Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Wed, 13 Jul 2016 07:38:01 +0900 Subject: [PATCH 5/7] Adjust the amount of memory allocation --- cpp/src/arrow/util/memory-pool-test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/util/memory-pool-test.cc b/cpp/src/arrow/util/memory-pool-test.cc index bdc66cde1cca4..109a4bc2becd1 100644 --- a/cpp/src/arrow/util/memory-pool-test.cc +++ b/cpp/src/arrow/util/memory-pool-test.cc @@ -50,8 +50,8 @@ TEST(DefaultMemoryPool, FreeLargeMemory) { MemoryPool* pool = default_memory_pool(); uint8_t* data; - ASSERT_OK(pool->Allocate(128, &data)); - ASSERT_EXIT(pool->Free(data, 256), ::testing::ExitedWithCode(1), + ASSERT_OK(pool->Allocate(100, &data)); + ASSERT_EXIT(pool->Free(data, 120), ::testing::ExitedWithCode(1), ".*Check failed: \\(bytes_allocated_\\) >= \\(size\\)"); } From f90313063969516f74e9145f4a2bac4149222876 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Wed, 13 Jul 2016 23:57:36 +0900 Subject: [PATCH 6/7] Free allocated memory after death --- cpp/src/arrow/util/memory-pool-test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/util/memory-pool-test.cc b/cpp/src/arrow/util/memory-pool-test.cc index 109a4bc2becd1..be2370f205db1 100644 --- a/cpp/src/arrow/util/memory-pool-test.cc +++ b/cpp/src/arrow/util/memory-pool-test.cc @@ -46,13 +46,14 @@ TEST(DefaultMemoryPool, OOM) { ASSERT_RAISES(OutOfMemory, pool->Allocate(to_alloc, &data)); } -TEST(DefaultMemoryPool, FreeLargeMemory) { +TEST(DefaultMemoryPoolDeathTest, FreeLargeMemory) { MemoryPool* pool = default_memory_pool(); uint8_t* data; ASSERT_OK(pool->Allocate(100, &data)); - ASSERT_EXIT(pool->Free(data, 120), ::testing::ExitedWithCode(1), + EXPECT_EXIT(pool->Free(data, 120), ::testing::ExitedWithCode(1), ".*Check failed: \\(bytes_allocated_\\) >= \\(size\\)"); + pool->Free(data, 100); } } // namespace arrow From cb9e7b13c10728b6aed9eb7186759c21fb1b7f3d Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 16 Jul 2016 00:57:22 +0900 Subject: [PATCH 7/7] Disable FreeLargeMemory test for release builds --- cpp/src/arrow/util/memory-pool-test.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/src/arrow/util/memory-pool-test.cc b/cpp/src/arrow/util/memory-pool-test.cc index be2370f205db1..919f3740982cf 100644 --- a/cpp/src/arrow/util/memory-pool-test.cc +++ b/cpp/src/arrow/util/memory-pool-test.cc @@ -51,8 +51,12 @@ TEST(DefaultMemoryPoolDeathTest, FreeLargeMemory) { uint8_t* data; ASSERT_OK(pool->Allocate(100, &data)); + +#ifndef NDEBUG EXPECT_EXIT(pool->Free(data, 120), ::testing::ExitedWithCode(1), ".*Check failed: \\(bytes_allocated_\\) >= \\(size\\)"); +#endif + pool->Free(data, 100); }