From 4d04c27f2bbfc397c4ed711a14f0e2b30ebef307 Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Mon, 4 Apr 2016 08:10:47 +0800 Subject: [PATCH 1/7] ARROW-85 memcmp can be avoided in Equal when comparing with the same Buffer --- cpp/src/arrow/util/buffer.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/util/buffer.h b/cpp/src/arrow/util/buffer.h index 94e53b61f2e..657b84ed27d 100644 --- a/cpp/src/arrow/util/buffer.h +++ b/cpp/src/arrow/util/buffer.h @@ -52,11 +52,13 @@ class Buffer : public std::enable_shared_from_this { // up to the number of compared bytes bool Equals(const Buffer& other, int64_t nbytes) const { return this == &other || (size_ >= nbytes && other.size_ >= nbytes && - !memcmp(data_, other.data_, nbytes)); + (data_ == other.data_ || !memcmp(data_, other.data_, nbytes))); } bool Equals(const Buffer& other) const { - return this == &other || (size_ == other.size_ && !memcmp(data_, other.data_, size_)); + return this == &other || + (size_ == other.size_ && (data_ == other.data_ || + !memcmp(data_, other.data_, size_))); } const uint8_t* data() const { return data_; } From 9f239a3c761338485ec642141e0f9e88f9a06c13 Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Mon, 4 Apr 2016 11:17:53 +0800 Subject: [PATCH 2/7] ARROW-85. Added tests for Buffer and the new behavior --- cpp/src/arrow/util/buffer-test.cc | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/cpp/src/arrow/util/buffer-test.cc b/cpp/src/arrow/util/buffer-test.cc index dad0f7461d9..b20682a2a27 100644 --- a/cpp/src/arrow/util/buffer-test.cc +++ b/cpp/src/arrow/util/buffer-test.cc @@ -53,4 +53,40 @@ TEST_F(TestBuffer, ResizeOOM) { ASSERT_RAISES(OutOfMemory, buf.Resize(to_alloc)); } +TEST_F(TestBuffer, EqualsWithSameContent) { + MemoryPool* pool = default_memory_pool(); + int32_t bufferSize = 128 * 1024; + uint8_t* rawBuffer1 = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer1)); + memset(rawBuffer1, 12, bufferSize); + uint8_t* rawBuffer2 = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer2)); + memset(rawBuffer2, 12, bufferSize); + uint8_t* rawBuffer3 = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer3)); + memset(rawBuffer3, 3, bufferSize); + + Buffer buffer1(rawBuffer1, bufferSize); + Buffer buffer2(rawBuffer2, bufferSize); + Buffer buffer3(rawBuffer3, bufferSize); + ASSERT_TRUE(buffer1.Equals(buffer2)); + ASSERT_FALSE(buffer1.Equals(buffer3)); +} + +TEST_F(TestBuffer, EqualsWithSameBuffer) { + MemoryPool* pool = default_memory_pool(); + int32_t bufferSize = 128 * 1024; + uint8_t* rawBuffer = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer)); + + Buffer buffer1(rawBuffer, bufferSize); + Buffer buffer2(rawBuffer, bufferSize); + ASSERT_TRUE(buffer1.Equals(buffer2)); + + int64_t nbytes = bufferSize / 2; + Buffer buffer3(rawBuffer, nbytes); + ASSERT_TRUE(buffer1.Equals(buffer3, nbytes)); + ASSERT_FALSE(buffer1.Equals(buffer3, nbytes + 1)); +} + } // namespace arrow From 1b486631e58ad232ecea7ff808e34d06c395be0d Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Mon, 4 Apr 2016 11:51:20 +0800 Subject: [PATCH 3/7] ARROW-85. Fixed checking styles --- cpp/src/arrow/util/buffer-test.cc | 2 +- cpp/src/arrow/util/buffer.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/util/buffer-test.cc b/cpp/src/arrow/util/buffer-test.cc index b20682a2a27..3a4b5d08877 100644 --- a/cpp/src/arrow/util/buffer-test.cc +++ b/cpp/src/arrow/util/buffer-test.cc @@ -84,7 +84,7 @@ TEST_F(TestBuffer, EqualsWithSameBuffer) { ASSERT_TRUE(buffer1.Equals(buffer2)); int64_t nbytes = bufferSize / 2; - Buffer buffer3(rawBuffer, nbytes); + Buffer buffer3(rawBuffer, nbytes); ASSERT_TRUE(buffer1.Equals(buffer3, nbytes)); ASSERT_FALSE(buffer1.Equals(buffer3, nbytes + 1)); } diff --git a/cpp/src/arrow/util/buffer.h b/cpp/src/arrow/util/buffer.h index 657b84ed27d..a7de06e53ef 100644 --- a/cpp/src/arrow/util/buffer.h +++ b/cpp/src/arrow/util/buffer.h @@ -56,7 +56,7 @@ class Buffer : public std::enable_shared_from_this { } bool Equals(const Buffer& other) const { - return this == &other || + return this == &other || (size_ == other.size_ && (data_ == other.data_ || !memcmp(data_, other.data_, size_))); } From 0ddcd018e6bd2f978d197edaf0f948a2d762ddff Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Mon, 4 Apr 2016 14:27:09 +0800 Subject: [PATCH 4/7] ARROW-85. Fixed another format issue --- cpp/src/arrow/util/buffer-test.cc | 34 +++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/cpp/src/arrow/util/buffer-test.cc b/cpp/src/arrow/util/buffer-test.cc index 3a4b5d08877..5ebfbf8f1c5 100644 --- a/cpp/src/arrow/util/buffer-test.cc +++ b/cpp/src/arrow/util/buffer-test.cc @@ -54,23 +54,23 @@ TEST_F(TestBuffer, ResizeOOM) { } TEST_F(TestBuffer, EqualsWithSameContent) { - MemoryPool* pool = default_memory_pool(); - int32_t bufferSize = 128 * 1024; - uint8_t* rawBuffer1 = NULL; - ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer1)); - memset(rawBuffer1, 12, bufferSize); - uint8_t* rawBuffer2 = NULL; - ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer2)); - memset(rawBuffer2, 12, bufferSize); - uint8_t* rawBuffer3 = NULL; - ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer3)); - memset(rawBuffer3, 3, bufferSize); - - Buffer buffer1(rawBuffer1, bufferSize); - Buffer buffer2(rawBuffer2, bufferSize); - Buffer buffer3(rawBuffer3, bufferSize); - ASSERT_TRUE(buffer1.Equals(buffer2)); - ASSERT_FALSE(buffer1.Equals(buffer3)); + MemoryPool* pool = default_memory_pool(); + int32_t bufferSize = 128 * 1024; + uint8_t* rawBuffer1 = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer1)); + memset(rawBuffer1, 12, bufferSize); + uint8_t* rawBuffer2 = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer2)); + memset(rawBuffer2, 12, bufferSize); + uint8_t* rawBuffer3 = NULL; + ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer3)); + memset(rawBuffer3, 3, bufferSize); + + Buffer buffer1(rawBuffer1, bufferSize); + Buffer buffer2(rawBuffer2, bufferSize); + Buffer buffer3(rawBuffer3, bufferSize); + ASSERT_TRUE(buffer1.Equals(buffer2)); + ASSERT_FALSE(buffer1.Equals(buffer3)); } TEST_F(TestBuffer, EqualsWithSameBuffer) { From b83f9893195d8f6474d7b6496339d2cec1b5053b Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Wed, 6 Apr 2016 06:07:39 +0800 Subject: [PATCH 5/7] ARROW-85. Corrected another format issue by clang-format --- cpp/src/arrow/util/buffer.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/util/buffer.h b/cpp/src/arrow/util/buffer.h index a7de06e53ef..56532be8070 100644 --- a/cpp/src/arrow/util/buffer.h +++ b/cpp/src/arrow/util/buffer.h @@ -51,14 +51,15 @@ class Buffer : public std::enable_shared_from_this { // Return true if both buffers are the same size and contain the same bytes // up to the number of compared bytes bool Equals(const Buffer& other, int64_t nbytes) const { - return this == &other || (size_ >= nbytes && other.size_ >= nbytes && - (data_ == other.data_ || !memcmp(data_, other.data_, nbytes))); + return this == &other || + (size_ >= nbytes && other.size_ >= nbytes && + (data_ == other.data_ || !memcmp(data_, other.data_, nbytes))); } bool Equals(const Buffer& other) const { return this == &other || - (size_ == other.size_ && (data_ == other.data_ || - !memcmp(data_, other.data_, size_))); + (size_ == other.size_ && + (data_ == other.data_ || !memcmp(data_, other.data_, size_))); } const uint8_t* data() const { return data_; } From 6a8bef53a1139d277d72eb8f5492cf2878946f9e Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Tue, 5 Apr 2016 08:42:09 +0800 Subject: [PATCH 6/7] Fixed some comments --- cpp/src/arrow/util/buffer-test.cc | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/util/buffer-test.cc b/cpp/src/arrow/util/buffer-test.cc index 5ebfbf8f1c5..23844e5c12d 100644 --- a/cpp/src/arrow/util/buffer-test.cc +++ b/cpp/src/arrow/util/buffer-test.cc @@ -55,14 +55,14 @@ TEST_F(TestBuffer, ResizeOOM) { TEST_F(TestBuffer, EqualsWithSameContent) { MemoryPool* pool = default_memory_pool(); - int32_t bufferSize = 128 * 1024; - uint8_t* rawBuffer1 = NULL; + const int32_t bufferSize = 128 * 1024; + uint8_t* rawBuffer1; ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer1)); memset(rawBuffer1, 12, bufferSize); - uint8_t* rawBuffer2 = NULL; + uint8_t* rawBuffer2; ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer2)); memset(rawBuffer2, 12, bufferSize); - uint8_t* rawBuffer3 = NULL; + uint8_t* rawBuffer3; ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer3)); memset(rawBuffer3, 3, bufferSize); @@ -75,15 +75,16 @@ TEST_F(TestBuffer, EqualsWithSameContent) { TEST_F(TestBuffer, EqualsWithSameBuffer) { MemoryPool* pool = default_memory_pool(); - int32_t bufferSize = 128 * 1024; - uint8_t* rawBuffer = NULL; + const int32_t bufferSize = 128 * 1024; + uint8_t* rawBuffer; ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer)); + memset(rawBuffer, 111, bufferSize); Buffer buffer1(rawBuffer, bufferSize); Buffer buffer2(rawBuffer, bufferSize); ASSERT_TRUE(buffer1.Equals(buffer2)); - int64_t nbytes = bufferSize / 2; + const int64_t nbytes = bufferSize / 2; Buffer buffer3(rawBuffer, nbytes); ASSERT_TRUE(buffer1.Equals(buffer3, nbytes)); ASSERT_FALSE(buffer1.Equals(buffer3, nbytes + 1)); From 2a70944390fa455fea24fc619c709a93183f09f6 Mon Sep 17 00:00:00 2001 From: Kai Zheng Date: Tue, 5 Apr 2016 09:54:09 +0800 Subject: [PATCH 7/7] Free test buffers afterwards --- cpp/src/arrow/util/buffer-test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cpp/src/arrow/util/buffer-test.cc b/cpp/src/arrow/util/buffer-test.cc index 23844e5c12d..cc4ec98e4fb 100644 --- a/cpp/src/arrow/util/buffer-test.cc +++ b/cpp/src/arrow/util/buffer-test.cc @@ -71,6 +71,10 @@ TEST_F(TestBuffer, EqualsWithSameContent) { Buffer buffer3(rawBuffer3, bufferSize); ASSERT_TRUE(buffer1.Equals(buffer2)); ASSERT_FALSE(buffer1.Equals(buffer3)); + + pool->Free(rawBuffer1, bufferSize); + pool->Free(rawBuffer2, bufferSize); + pool->Free(rawBuffer3, bufferSize); } TEST_F(TestBuffer, EqualsWithSameBuffer) { @@ -88,6 +92,8 @@ TEST_F(TestBuffer, EqualsWithSameBuffer) { Buffer buffer3(rawBuffer, nbytes); ASSERT_TRUE(buffer1.Equals(buffer3, nbytes)); ASSERT_FALSE(buffer1.Equals(buffer3, nbytes + 1)); + + pool->Free(rawBuffer, bufferSize); } } // namespace arrow