From 375fc48a06405bddd9472d440b194c98928741f4 Mon Sep 17 00:00:00 2001 From: Jim Apple Date: Tue, 7 Aug 2018 09:19:56 -0700 Subject: [PATCH 1/5] ARROW-2998: [C++] Add unique_ptr versions of Allocate[Resizable]Buffer --- cpp/src/arrow/buffer.cc | 28 ++++++++++++++++++++++++++++ cpp/src/arrow/buffer.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/cpp/src/arrow/buffer.cc b/cpp/src/arrow/buffer.cc index 91b0002fcc2..d3eda322f69 100644 --- a/cpp/src/arrow/buffer.cc +++ b/cpp/src/arrow/buffer.cc @@ -155,10 +155,23 @@ Status AllocateBuffer(MemoryPool* pool, const int64_t size, return Status::OK(); } +Status AllocateBuffer(MemoryPool* pool, const int64_t size, + std::unique_ptr* out) { + auto buffer = std::unique_ptr(new PoolBuffer(pool)); + RETURN_NOT_OK(buffer->Resize(size)); + buffer->ZeroPadding(); + *out = std::move(buffer); + return Status::OK(); +} + Status AllocateBuffer(const int64_t size, std::shared_ptr* out) { return AllocateBuffer(default_memory_pool(), size, out); } +Status AllocateBuffer(const int64_t size, std::unique_ptr* out) { + return AllocateBuffer(default_memory_pool(), size, out); +} + Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out) { auto buffer = std::make_shared(pool); @@ -168,11 +181,26 @@ Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, return Status::OK(); } +Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, + std::unique_ptr* out) { + auto buffer = std::unique_ptr(new PoolBuffer(pool)); + RETURN_NOT_OK(buffer->Resize(size)); + buffer->ZeroPadding(); + *out = std::move(buffer); + return Status::OK(); +} + Status AllocateResizableBuffer(const int64_t size, std::shared_ptr* out) { return AllocateResizableBuffer(default_memory_pool(), size, out); } +Status AllocateResizableBuffer(const int64_t size, + std::unique_ptr* out) { + return AllocateResizableBuffer(default_memory_pool(), size, out); +} + + Status AllocateEmptyBitmap(MemoryPool* pool, int64_t length, std::shared_ptr* out) { RETURN_NOT_OK(AllocateBuffer(pool, BitUtil::BytesForBits(length), out)); diff --git a/cpp/src/arrow/buffer.h b/cpp/src/arrow/buffer.h index 486f04633b9..7bcfa64000b 100644 --- a/cpp/src/arrow/buffer.h +++ b/cpp/src/arrow/buffer.h @@ -231,6 +231,16 @@ class ARROW_EXPORT ResizableBuffer : public MutableBuffer { ARROW_EXPORT Status AllocateBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out); +/// \brief Allocate a fixed size mutable buffer from a memory pool, zero its padding. +/// +/// \param[in] pool a memory pool +/// \param[in] size size of buffer to allocate +/// \param[out] out the allocated buffer (contains padding) +/// +/// \return Status message +ARROW_EXPORT +Status AllocateBuffer(MemoryPool* pool, const int64_t size, std::unique_ptr* out); + /// \brief Allocate a fixed-size mutable buffer from the default memory pool /// /// \param[in] size size of buffer to allocate @@ -240,6 +250,15 @@ Status AllocateBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out); +/// \brief Allocate a fixed-size mutable buffer from the default memory pool +/// +/// \param[in] size size of buffer to allocate +/// \param[out] out the allocated buffer (contains padding) +/// +/// \return Status message +ARROW_EXPORT +Status AllocateBuffer(const int64_t size, std::unique_ptr* out); + /// \brief Allocate a resizeable buffer from a memory pool, zero its padding. /// /// \param[in] pool a memory pool @@ -251,6 +270,17 @@ ARROW_EXPORT Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out); +/// \brief Allocate a resizeable buffer from a memory pool, zero its padding. +/// +/// \param[in] pool a memory pool +/// \param[in] size size of buffer to allocate +/// \param[out] out the allocated buffer +/// +/// \return Status message +ARROW_EXPORT +Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, + std::unique_ptr* out); + /// \brief Allocate a resizeable buffer from the default memory pool /// /// \param[in] size size of buffer to allocate @@ -260,6 +290,16 @@ Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, ARROW_EXPORT Status AllocateResizableBuffer(const int64_t size, std::shared_ptr* out); + +/// \brief Allocate a resizeable buffer from the default memory pool +/// +/// \param[in] size size of buffer to allocate +/// \param[out] out the allocated buffer +/// +/// \return Status message +ARROW_EXPORT +Status AllocateResizableBuffer(const int64_t size, std::unique_ptr* out); + /// \brief Allocate a zero-initialized bitmap buffer from a memory pool /// /// \param[in] pool memory pool to allocate memory from From 63eb608bfd58902a1a63b789855a01a548c88e71 Mon Sep 17 00:00:00 2001 From: Jim Apple Date: Tue, 7 Aug 2018 16:49:10 -0700 Subject: [PATCH 2/5] lint fix: #include for std::move --- cpp/src/arrow/buffer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/src/arrow/buffer.cc b/cpp/src/arrow/buffer.cc index d3eda322f69..7cb544e1ecf 100644 --- a/cpp/src/arrow/buffer.cc +++ b/cpp/src/arrow/buffer.cc @@ -18,6 +18,7 @@ #include "arrow/buffer.h" #include +#include #include "arrow/memory_pool.h" #include "arrow/status.h" From 7bad045e5dfb513baab9f62aee7f4f01a62c0d2b Mon Sep 17 00:00:00 2001 From: Jim Apple Date: Wed, 8 Aug 2018 09:59:46 -0700 Subject: [PATCH 3/5] Reduce code duplication in buffer.cc --- cpp/src/arrow/buffer.cc | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/cpp/src/arrow/buffer.cc b/cpp/src/arrow/buffer.cc index 7cb544e1ecf..da0a1929751 100644 --- a/cpp/src/arrow/buffer.cc +++ b/cpp/src/arrow/buffer.cc @@ -147,22 +147,28 @@ MutableBuffer::MutableBuffer(const std::shared_ptr& parent, const int64_ parent_ = parent; } -Status AllocateBuffer(MemoryPool* pool, const int64_t size, - std::shared_ptr* out) { - auto buffer = std::make_shared(pool); +namespace { +// A utility that does most of the work of the `AllocateBuffer` and +// `AllocateResizableBuffer` methods. The argument `buffer` should be a smart pointer to a +// PoolBuffer. +template +inline Status ResizePoolBuffer(PoolBufferPtr&& buffer, const int64_t size, + BufferPtr* out) { RETURN_NOT_OK(buffer->Resize(size)); buffer->ZeroPadding(); - *out = buffer; + *out = std::move(buffer); return Status::OK(); } +} // namespace + +Status AllocateBuffer(MemoryPool* pool, const int64_t size, + std::shared_ptr* out) { + return ResizePoolBuffer(std::make_shared(pool), size, out); +} Status AllocateBuffer(MemoryPool* pool, const int64_t size, std::unique_ptr* out) { - auto buffer = std::unique_ptr(new PoolBuffer(pool)); - RETURN_NOT_OK(buffer->Resize(size)); - buffer->ZeroPadding(); - *out = std::move(buffer); - return Status::OK(); + return ResizePoolBuffer(std::unique_ptr(new PoolBuffer(pool)), size, out); } Status AllocateBuffer(const int64_t size, std::shared_ptr* out) { @@ -175,20 +181,12 @@ Status AllocateBuffer(const int64_t size, std::unique_ptr* out) { Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out) { - auto buffer = std::make_shared(pool); - RETURN_NOT_OK(buffer->Resize(size)); - buffer->ZeroPadding(); - *out = buffer; - return Status::OK(); + return ResizePoolBuffer(std::make_shared(pool), size, out); } Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, std::unique_ptr* out) { - auto buffer = std::unique_ptr(new PoolBuffer(pool)); - RETURN_NOT_OK(buffer->Resize(size)); - buffer->ZeroPadding(); - *out = std::move(buffer); - return Status::OK(); + return ResizePoolBuffer(std::unique_ptr(new PoolBuffer(pool)), size, out); } Status AllocateResizableBuffer(const int64_t size, From f1a086b3519b6c799813e88cd12d12568c6d7d0d Mon Sep 17 00:00:00 2001 From: Jim Apple Date: Wed, 8 Aug 2018 10:00:35 -0700 Subject: [PATCH 4/5] Rearrange and refactor buffer-test.cc to use both unique and shared ptrs --- cpp/src/arrow/buffer-test.cc | 146 ++++++++++++++++++----------------- 1 file changed, 77 insertions(+), 69 deletions(-) diff --git a/cpp/src/arrow/buffer-test.cc b/cpp/src/arrow/buffer-test.cc index 55b86e96ea4..368a9cb0138 100644 --- a/cpp/src/arrow/buffer-test.cc +++ b/cpp/src/arrow/buffer-test.cc @@ -32,19 +32,6 @@ using std::string; namespace arrow { -TEST(TestBuffer, IsMutableFlag) { - Buffer buf(nullptr, 0); - - ASSERT_FALSE(buf.is_mutable()); - - MutableBuffer mbuf(nullptr, 0); - ASSERT_TRUE(mbuf.is_mutable()); - - std::shared_ptr pool_buf; - ASSERT_OK(AllocateResizableBuffer(0, &pool_buf)); - ASSERT_TRUE(pool_buf->is_mutable()); -} - TEST(TestBuffer, FromStdString) { std::string val = "hello, world"; @@ -70,62 +57,6 @@ TEST(TestBuffer, FromStdStringWithMemory) { ASSERT_EQ(static_cast(expected.size()), buf->size()); } -TEST(TestBuffer, Resize) { - std::shared_ptr buf; - ASSERT_OK(AllocateResizableBuffer(0, &buf)); - - ASSERT_EQ(0, buf->size()); - ASSERT_OK(buf->Resize(100)); - ASSERT_EQ(100, buf->size()); - ASSERT_OK(buf->Resize(200)); - ASSERT_EQ(200, buf->size()); - - // Make it smaller, too - ASSERT_OK(buf->Resize(50, true)); - ASSERT_EQ(50, buf->size()); - // We have actually shrunken in size - // The spec requires that capacity is a multiple of 64 - ASSERT_EQ(64, buf->capacity()); - - // Resize to a larger capacity again to test shrink_to_fit = false - ASSERT_OK(buf->Resize(100)); - ASSERT_EQ(128, buf->capacity()); - ASSERT_OK(buf->Resize(50, false)); - ASSERT_EQ(128, buf->capacity()); -} - -TEST(TestBuffer, TypedResize) { - std::shared_ptr buf; - ASSERT_OK(AllocateResizableBuffer(0, &buf)); - - ASSERT_EQ(0, buf->size()); - ASSERT_OK(buf->TypedResize(100)); - ASSERT_EQ(800, buf->size()); - ASSERT_OK(buf->TypedResize(200)); - ASSERT_EQ(1600, buf->size()); - - ASSERT_OK(buf->TypedResize(50, true)); - ASSERT_EQ(400, buf->size()); - ASSERT_EQ(448, buf->capacity()); - - ASSERT_OK(buf->TypedResize(100)); - ASSERT_EQ(832, buf->capacity()); - ASSERT_OK(buf->TypedResize(50, false)); - ASSERT_EQ(832, buf->capacity()); -} - -TEST(TestBuffer, ResizeOOM) { -// This test doesn't play nice with AddressSanitizer -#ifndef ADDRESS_SANITIZER - // realloc fails, even though there may be no explicit limit - std::shared_ptr buf; - ASSERT_OK(AllocateResizableBuffer(0, &buf)); - ASSERT_OK(buf->Resize(100)); - int64_t to_alloc = std::numeric_limits::max(); - ASSERT_RAISES(OutOfMemory, buf->Resize(to_alloc)); -#endif -} - TEST(TestBuffer, EqualsWithSameContent) { MemoryPool* pool = default_memory_pool(); const int32_t bufferSize = 128 * 1024; @@ -243,4 +174,81 @@ TEST(TestBufferBuilder, ResizeReserve) { ASSERT_EQ(128, builder.capacity()); } +template +class TypedTestBuffer : public ::testing::Test {}; + +using BufferPtrs = + ::testing::Types, std::unique_ptr>; + +TYPED_TEST_CASE(TypedTestBuffer, BufferPtrs); + +TYPED_TEST(TypedTestBuffer, IsMutableFlag) { + Buffer buf(nullptr, 0); + + ASSERT_FALSE(buf.is_mutable()); + + MutableBuffer mbuf(nullptr, 0); + ASSERT_TRUE(mbuf.is_mutable()); + + TypeParam pool_buf; + ASSERT_OK(AllocateResizableBuffer(0, &pool_buf)); + ASSERT_TRUE(pool_buf->is_mutable()); +} + +TYPED_TEST(TypedTestBuffer, Resize) { + TypeParam buf; + ASSERT_OK(AllocateResizableBuffer(0, &buf)); + + ASSERT_EQ(0, buf->size()); + ASSERT_OK(buf->Resize(100)); + ASSERT_EQ(100, buf->size()); + ASSERT_OK(buf->Resize(200)); + ASSERT_EQ(200, buf->size()); + + // Make it smaller, too + ASSERT_OK(buf->Resize(50, true)); + ASSERT_EQ(50, buf->size()); + // We have actually shrunken in size + // The spec requires that capacity is a multiple of 64 + ASSERT_EQ(64, buf->capacity()); + + // Resize to a larger capacity again to test shrink_to_fit = false + ASSERT_OK(buf->Resize(100)); + ASSERT_EQ(128, buf->capacity()); + ASSERT_OK(buf->Resize(50, false)); + ASSERT_EQ(128, buf->capacity()); +} + +TYPED_TEST(TypedTestBuffer, TypedResize) { + TypeParam buf; + ASSERT_OK(AllocateResizableBuffer(0, &buf)); + + ASSERT_EQ(0, buf->size()); + ASSERT_OK(buf->template TypedResize(100)); + ASSERT_EQ(800, buf->size()); + ASSERT_OK(buf->template TypedResize(200)); + ASSERT_EQ(1600, buf->size()); + + ASSERT_OK(buf->template TypedResize(50, true)); + ASSERT_EQ(400, buf->size()); + ASSERT_EQ(448, buf->capacity()); + + ASSERT_OK(buf->template TypedResize(100)); + ASSERT_EQ(832, buf->capacity()); + ASSERT_OK(buf->template TypedResize(50, false)); + ASSERT_EQ(832, buf->capacity()); +} + +TYPED_TEST(TypedTestBuffer, ResizeOOM) { +// This test doesn't play nice with AddressSanitizer +#ifndef ADDRESS_SANITIZER + // realloc fails, even though there may be no explicit limit + TypeParam buf; + ASSERT_OK(AllocateResizableBuffer(0, &buf)); + ASSERT_OK(buf->Resize(100)); + int64_t to_alloc = std::numeric_limits::max(); + ASSERT_RAISES(OutOfMemory, buf->Resize(to_alloc)); +#endif +} + } // namespace arrow From b157dedcabbf7e0aa93d0efc8f3fa647e2d2a73e Mon Sep 17 00:00:00 2001 From: Jim Apple Date: Wed, 8 Aug 2018 13:06:30 -0700 Subject: [PATCH 5/5] Fix make check-format: remove unneeded empty lines --- cpp/src/arrow/buffer.cc | 1 - cpp/src/arrow/buffer.h | 1 - 2 files changed, 2 deletions(-) diff --git a/cpp/src/arrow/buffer.cc b/cpp/src/arrow/buffer.cc index da0a1929751..388e98f56ca 100644 --- a/cpp/src/arrow/buffer.cc +++ b/cpp/src/arrow/buffer.cc @@ -199,7 +199,6 @@ Status AllocateResizableBuffer(const int64_t size, return AllocateResizableBuffer(default_memory_pool(), size, out); } - Status AllocateEmptyBitmap(MemoryPool* pool, int64_t length, std::shared_ptr* out) { RETURN_NOT_OK(AllocateBuffer(pool, BitUtil::BytesForBits(length), out)); diff --git a/cpp/src/arrow/buffer.h b/cpp/src/arrow/buffer.h index 7bcfa64000b..442c4516985 100644 --- a/cpp/src/arrow/buffer.h +++ b/cpp/src/arrow/buffer.h @@ -290,7 +290,6 @@ Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, ARROW_EXPORT Status AllocateResizableBuffer(const int64_t size, std::shared_ptr* out); - /// \brief Allocate a resizeable buffer from the default memory pool /// /// \param[in] size size of buffer to allocate