Skip to content

ARROW-85: memcmp can be avoided in Equal when comparing with the same … #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

ARROW-85: memcmp can be avoided in Equal when comparing with the same … #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d04c27
ARROW-85 memcmp can be avoided in Equal when comparing with the same …
Apr 4, 2016
9f239a3
ARROW-85. Added tests for Buffer and the new behavior
Apr 4, 2016
1b48663
ARROW-85. Fixed checking styles
Apr 4, 2016
0ddcd01
ARROW-85. Fixed another format issue
Apr 4, 2016
b83f989
ARROW-85. Corrected another format issue by clang-format
Apr 5, 2016
6a8bef5
Fixed some comments
Apr 5, 2016
2a70944
Free test buffers afterwards
Apr 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions cpp/src/arrow/util/buffer-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,47 @@ TEST_F(TestBuffer, ResizeOOM) {
ASSERT_RAISES(OutOfMemory, buf.Resize(to_alloc));
}

TEST_F(TestBuffer, EqualsWithSameContent) {
MemoryPool* pool = default_memory_pool();
const int32_t bufferSize = 128 * 1024;
uint8_t* rawBuffer1;
ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer1));
memset(rawBuffer1, 12, bufferSize);
uint8_t* rawBuffer2;
ASSERT_OK(pool->Allocate(bufferSize, &rawBuffer2));
memset(rawBuffer2, 12, bufferSize);
uint8_t* rawBuffer3;
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));

pool->Free(rawBuffer1, bufferSize);
pool->Free(rawBuffer2, bufferSize);
pool->Free(rawBuffer3, bufferSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change these tests to use PoolBuffer instead of Buffer (and do so in the future)? That way memory will be automatically deallocated in destructors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late and thanks Wes. I just attempted to change the codes using PoolBuffer, and found that the current codes are more readable regarding the test logic. I do agree for most tests that need buffer, PoolBuffer would be better to save the free. Sure I will use it in the future.

}

TEST_F(TestBuffer, EqualsWithSameBuffer) {
MemoryPool* pool = default_memory_pool();
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));

const int64_t nbytes = bufferSize / 2;
Buffer buffer3(rawBuffer, nbytes);
ASSERT_TRUE(buffer1.Equals(buffer3, nbytes));
ASSERT_FALSE(buffer1.Equals(buffer3, nbytes + 1));

pool->Free(rawBuffer, bufferSize);
}

} // namespace arrow
9 changes: 6 additions & 3 deletions cpp/src/arrow/util/buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,15 @@ class Buffer : public std::enable_shared_from_this<Buffer> {
// 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 &&
!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_ && !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_; }
Expand Down