Skip to content
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

ARROW-238: Change InternalMemoryPool::Free() to return Status::Invalid when ther… #102

Closed
wants to merge 7 commits into from
Closed

ARROW-238: Change InternalMemoryPool::Free() to return Status::Invalid when ther… #102

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
7651570
Change InternalMemoryPool::Free() to return Status::Invalid when ther…
jihoonson Jul 11, 2016
e89a1f9
Change python implementation as well.
jihoonson Jul 11, 2016
b4159f0
Reflect comments
jihoonson Jul 12, 2016
b1af59b
Change to ASSERT_EXIT
jihoonson Jul 12, 2016
0077a70
Adjust the amount of memory allocation
jihoonson Jul 12, 2016
f903130
Free allocated memory after death
jihoonson Jul 13, 2016
cb9e7b1
Disable FreeLargeMemory test for release builds
jihoonson Jul 15, 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
2 changes: 1 addition & 1 deletion cpp/src/arrow/util/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions cpp/src/arrow/util/memory-pool-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,18 @@ TEST(DefaultMemoryPool, OOM) {
ASSERT_RAISES(OutOfMemory, pool->Allocate(to_alloc, &data));
}

TEST(DefaultMemoryPoolDeathTest, FreeLargeMemory) {
MemoryPool* pool = default_memory_pool();

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);
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 disable this test for release builds? See the NDEBUG define in CMakeLists.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment. Actually, I'm not an expert of C++, and would like to ask a question which may sound silly.
Does your comment mean that this unit test needs to be disabled for release builds? or, DCHECK in Free() should be disabled?
Would you please give me some backgrounds around this briefly?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/logging.h#L45

When compiled in release mode, the DCHECK statements are omitted. Thus, the unit test will not pass. You can try it yourself by passing -DCMAKE_BUILD_TYPE=release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I disabled the test for release builds.

}

} // namespace arrow
2 changes: 2 additions & 0 deletions cpp/src/arrow/util/memory-pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <sstream>

#include "arrow/util/status.h"
#include "arrow/util/logging.h"

namespace arrow {

Expand Down Expand Up @@ -81,6 +82,7 @@ int64_t InternalMemoryPool::bytes_allocated() const {

void InternalMemoryPool::Free(uint8_t* buffer, int64_t size) {
std::lock_guard<std::mutex> guard(pool_lock_);
DCHECK_GE(bytes_allocated_, size);
std::free(buffer);
bytes_allocated_ -= size;
}
Expand Down