You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ap...@apache.org on 2019/01/03 20:53:05 UTC
[arrow] branch master updated: ARROW-4009: [CI] Run Valgrind and
C++ code coverage in different builds
This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 7f1fbf8 ARROW-4009: [CI] Run Valgrind and C++ code coverage in different builds
7f1fbf8 is described below
commit 7f1fbf83284b745ee9215f6722e114ee467bdeb8
Author: Antoine Pitrou <an...@python.org>
AuthorDate: Thu Jan 3 21:52:57 2019 +0100
ARROW-4009: [CI] Run Valgrind and C++ code coverage in different builds
Also fix a couple ASAN / UBSAN issues (not all of them).
Author: Antoine Pitrou <an...@python.org>
Closes #3302 from pitrou/ARROW-4009-separate-codecov-valgrind and squashes the following commits:
7dacf9e1 <Antoine Pitrou> Mark function inline
06372930 <Antoine Pitrou> Use clang on non-coverage C++ job
eca54b69 <Antoine Pitrou> ARROW-4009: Run Valgrind and C++ code coverage in different builds
---
.travis.yml | 30 ++++-
cpp/CMakeLists.txt | 8 ++
cpp/cmake_modules/san-config.cmake | 24 +---
cpp/src/arrow/array-test.cc | 13 ++-
cpp/src/arrow/array/builder_binary.h | 1 +
cpp/src/arrow/array/builder_nested.h | 1 +
cpp/src/arrow/buffer-builder.h | 205 +++++++++++++++++++++++++++++++++
cpp/src/arrow/buffer-test.cc | 1 +
cpp/src/arrow/buffer.cc | 21 ++--
cpp/src/arrow/buffer.h | 168 +--------------------------
cpp/src/arrow/io/readahead.cc | 2 +
cpp/src/arrow/memory_pool.cc | 79 ++++++++-----
cpp/src/arrow/test-util.cc | 16 ++-
cpp/src/arrow/util/thread-pool-test.cc | 2 +-
cpp/src/parquet/bloom_filter.h | 1 +
15 files changed, 324 insertions(+), 248 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index 059daee..837b4cf 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -61,13 +61,35 @@ matrix:
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
script:
- $TRAVIS_BUILD_DIR/ci/travis_lint.sh
- - name: "C++ unit tests, code coverage with gcc 4.8"
+ - name: "C++ unit tests w/ Valgrind, clang 6.0"
+ language: cpp
+ os: linux
+ env:
+ - ARROW_TRAVIS_VALGRIND=1
+ - ARROW_TRAVIS_USE_TOOLCHAIN=1
+ - ARROW_TRAVIS_PLASMA=1
+ - ARROW_TRAVIS_ORC=1
+ - ARROW_TRAVIS_PARQUET=1
+ - ARROW_TRAVIS_GANDIVA=1
+ - ARROW_BUILD_WARNING_LEVEL=CHECKIN
+ - CC="clang-6.0"
+ - CXX="clang++-6.0"
+ before_script:
+ - if [ $ARROW_CI_CPP_AFFECTED != "1" ]; then exit; fi
+ - $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
+ - $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
+ # If either C++ or Python changed, we must install the C++ libraries
+ - git submodule update --init
+ - $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh
+ script:
+ - $TRAVIS_BUILD_DIR/ci/travis_script_cpp.sh || travis_terminate 1
+ # Separating Valgrind and C++ coverage makes individual jobs shorter
+ - name: "C++ unit tests w/ gcc 4.8, coverage"
compiler: gcc
language: cpp
os: linux
jdk: openjdk8
env:
- - ARROW_TRAVIS_VALGRIND=1
- ARROW_TRAVIS_USE_TOOLCHAIN=1
- ARROW_TRAVIS_PLASMA=1
- ARROW_TRAVIS_ORC=1
@@ -87,7 +109,7 @@ matrix:
- $TRAVIS_BUILD_DIR/ci/travis_script_cpp.sh || travis_terminate 1
- $TRAVIS_BUILD_DIR/ci/travis_script_gandiva_java.sh || travis_terminate 1
- $TRAVIS_BUILD_DIR/ci/travis_upload_cpp_coverage.sh || travis_terminate 1
- - name: "Python 2.7 and 3.6 unit tests, coverage with gcc 4.8"
+ - name: "Python 2.7 and 3.6 unit tests w/ Valgrind, gcc 4.8, coverage"
compiler: gcc
language: cpp
os: linux
@@ -98,10 +120,10 @@ matrix:
- ARROW_TRAVIS_USE_TOOLCHAIN=1
- ARROW_TRAVIS_COVERAGE=1
- ARROW_TRAVIS_PYTHON_DOCS=1
- - ARROW_BUILD_WARNING_LEVEL=CHECKIN
- ARROW_TRAVIS_PYTHON_JVM=1
- ARROW_TRAVIS_PYTHON_GANDIVA=1
- ARROW_TRAVIS_OPTIONAL_INSTALL=1
+ - ARROW_BUILD_WARNING_LEVEL=CHECKIN
# TODO(wesm): Run the benchmarks outside of Travis
# - ARROW_TRAVIS_PYTHON_BENCHMARKS=1
before_script:
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index 006b406..aba1a59 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -111,6 +111,14 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")
"Run the test suite using valgrind --tool=memcheck"
OFF)
+ option(ARROW_USE_ASAN
+ "Enable Address Sanitizer checks"
+ OFF)
+
+ option(ARROW_USE_TSAN
+ "Enable Thread Sanitizer checks"
+ OFF)
+
option(ARROW_BUILD_TESTS
"Build the Arrow googletest unit tests, default OFF"
OFF)
diff --git a/cpp/cmake_modules/san-config.cmake b/cpp/cmake_modules/san-config.cmake
index f2de9cf..22a9b0c 100644
--- a/cpp/cmake_modules/san-config.cmake
+++ b/cpp/cmake_modules/san-config.cmake
@@ -22,19 +22,6 @@ if (${ARROW_USE_ASAN})
("${COMPILER_FAMILY}" STREQUAL "gcc" AND "${COMPILER_VERSION}" VERSION_GREATER "4.8")))
message(SEND_ERROR "Cannot use ASAN without clang or gcc >= 4.8")
endif()
-
- # If UBSAN is also enabled, and we're on clang < 3.5, ensure static linking is
- # enabled. Otherwise, we run into https://llvm.org/bugs/show_bug.cgi?id=18211
- if("${ARROW_USE_UBSAN}" AND
- "${COMPILER_FAMILY}" STREQUAL "clang" AND
- "${COMPILER_VERSION}" VERSION_LESS "3.5")
- if("${ARROW_LINK}" STREQUAL "a")
- message("Using static linking for ASAN+UBSAN build")
- set(ARROW_LINK "s")
- elseif("${ARROW_LINK}" STREQUAL "d")
- message(SEND_ERROR "Cannot use dynamic linking when ASAN and UBSAN are both enabled")
- endif()
- endif()
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -DADDRESS_SANITIZER")
endif()
@@ -49,7 +36,7 @@ if (${ARROW_USE_UBSAN})
("${COMPILER_FAMILY}" STREQUAL "gcc" AND "${COMPILER_VERSION}" VERSION_GREATER "4.9")))
message(SEND_ERROR "Cannot use UBSAN without clang or gcc >= 4.9")
endif()
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined -fno-sanitize=alignment,vptr -fno-sanitize-recover")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined -fno-sanitize=alignment,vptr -fno-sanitize-recover=all")
endif ()
# Flag to enable thread sanitizer (clang or gcc 4.8)
@@ -101,14 +88,7 @@ if ("${ARROW_USE_UBSAN}" OR "${ARROW_USE_ASAN}" OR "${ARROW_USE_TSAN}")
# GCC 4.8 and 4.9 (latest as of this writing) don't allow you to specify a
# sanitizer blacklist.
if("${COMPILER_FAMILY}" STREQUAL "clang")
- # Require clang 3.4 or newer; clang 3.3 has issues with TSAN and pthread
- # symbol interception.
- if("${COMPILER_VERSION}" VERSION_LESS "3.4")
- message(SEND_ERROR "Must use clang 3.4 or newer to run a sanitizer build."
- " Detected unsupported version ${COMPILER_VERSION}."
- " Try using clang from $NATIVE_TOOLCHAIN/.")
- endif()
- add_definitions("-fsanitize-blacklist=${BUILD_SUPPORT_DIR}/sanitize-blacklist.txt")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize-blacklist=${BUILD_SUPPORT_DIR}/sanitize-blacklist.txt")
else()
message(WARNING "GCC does not support specifying a sanitizer blacklist. Known sanitizer check failures will not be suppressed.")
endif()
diff --git a/cpp/src/arrow/array-test.cc b/cpp/src/arrow/array-test.cc
index bdb7eda..ccdaad5 100644
--- a/cpp/src/arrow/array-test.cc
+++ b/cpp/src/arrow/array-test.cc
@@ -30,6 +30,7 @@
#include <gtest/gtest.h>
#include "arrow/array.h"
+#include "arrow/buffer-builder.h"
#include "arrow/buffer.h"
#include "arrow/builder.h"
#include "arrow/ipc/test-common.h"
@@ -761,22 +762,22 @@ TYPED_TEST(TestPrimitiveBuilder, TestAppendValuesLazyIter) {
auto& draws = this->draws_;
auto& valid_bytes = this->valid_bytes_;
- auto doubler = [&draws](int64_t index) { return draws[index] * 2; };
- auto lazy_iter = internal::MakeLazyRange(doubler, size);
+ auto halve = [&draws](int64_t index) { return draws[index] / 2; };
+ auto lazy_iter = internal::MakeLazyRange(halve, size);
ASSERT_OK(this->builder_->AppendValues(lazy_iter.begin(), lazy_iter.end(),
valid_bytes.begin()));
- std::vector<T> doubled;
- transform(draws.begin(), draws.end(), back_inserter(doubled),
- [](T in) { return in * 2; });
+ std::vector<T> halved;
+ transform(draws.begin(), draws.end(), back_inserter(halved),
+ [](T in) { return in / 2; });
std::shared_ptr<Array> result;
FinishAndCheckPadding(this->builder_.get(), &result);
std::shared_ptr<Array> expected;
ASSERT_OK(
- this->builder_->AppendValues(doubled.data(), doubled.size(), valid_bytes.data()));
+ this->builder_->AppendValues(halved.data(), halved.size(), valid_bytes.data()));
FinishAndCheckPadding(this->builder_.get(), &expected);
ASSERT_TRUE(expected->Equals(result));
diff --git a/cpp/src/arrow/array/builder_binary.h b/cpp/src/arrow/array/builder_binary.h
index 7c101bd..324279d 100644
--- a/cpp/src/arrow/array/builder_binary.h
+++ b/cpp/src/arrow/array/builder_binary.h
@@ -25,6 +25,7 @@
#include "arrow/array.h"
#include "arrow/array/builder_base.h"
+#include "arrow/buffer-builder.h"
#include "arrow/status.h"
#include "arrow/type_traits.h"
#include "arrow/util/macros.h"
diff --git a/cpp/src/arrow/array/builder_nested.h b/cpp/src/arrow/array/builder_nested.h
index 863e6fe..19b0ad8 100644
--- a/cpp/src/arrow/array/builder_nested.h
+++ b/cpp/src/arrow/array/builder_nested.h
@@ -21,6 +21,7 @@
#include <vector>
#include "arrow/array/builder_base.h"
+#include "arrow/buffer-builder.h"
namespace arrow {
diff --git a/cpp/src/arrow/buffer-builder.h b/cpp/src/arrow/buffer-builder.h
new file mode 100644
index 0000000..dafa3ee
--- /dev/null
+++ b/cpp/src/arrow/buffer-builder.h
@@ -0,0 +1,205 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#ifndef ARROW_BUFFER_BUILDER_H
+#define ARROW_BUFFER_BUILDER_H
+
+#include <algorithm>
+#include <array>
+#include <cstdint>
+#include <cstring>
+#include <memory>
+#include <string>
+
+#include "arrow/buffer.h"
+#include "arrow/status.h"
+#include "arrow/util/bit-util.h"
+#include "arrow/util/macros.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow {
+
+// ----------------------------------------------------------------------
+// Buffer builder classes
+
+/// \class BufferBuilder
+/// \brief A class for incrementally building a contiguous chunk of in-memory data
+class ARROW_EXPORT BufferBuilder {
+ public:
+ explicit BufferBuilder(MemoryPool* pool ARROW_MEMORY_POOL_DEFAULT)
+ : pool_(pool), data_(NULLPTR), capacity_(0), size_(0) {}
+
+ /// \brief Resize the buffer to the nearest multiple of 64 bytes
+ ///
+ /// \param elements the new capacity of the of the builder. Will be rounded
+ /// up to a multiple of 64 bytes for padding
+ /// \param shrink_to_fit if new capacity is smaller than the existing size,
+ /// reallocate internal buffer. Set to false to avoid reallocations when
+ /// shrinking the builder.
+ /// \return Status
+ Status Resize(const int64_t elements, bool shrink_to_fit = true) {
+ // Resize(0) is a no-op
+ if (elements == 0) {
+ return Status::OK();
+ }
+ int64_t old_capacity = capacity_;
+
+ if (buffer_ == NULLPTR) {
+ ARROW_RETURN_NOT_OK(AllocateResizableBuffer(pool_, elements, &buffer_));
+ } else {
+ ARROW_RETURN_NOT_OK(buffer_->Resize(elements, shrink_to_fit));
+ }
+ capacity_ = buffer_->capacity();
+ data_ = buffer_->mutable_data();
+ if (capacity_ > old_capacity) {
+ memset(data_ + old_capacity, 0, capacity_ - old_capacity);
+ }
+ return Status::OK();
+ }
+
+ /// \brief Ensure that builder can accommodate the additional number of bytes
+ /// without the need to perform allocations
+ ///
+ /// \param size number of additional bytes to make space for
+ /// \return Status
+ Status Reserve(const int64_t size) { return Resize(size_ + size, false); }
+
+ /// \brief Append the given data to the buffer
+ ///
+ /// The buffer is automatically expanded if necessary.
+ Status Append(const void* data, int64_t length) {
+ if (capacity_ < length + size_) {
+ int64_t new_capacity = BitUtil::NextPower2(length + size_);
+ ARROW_RETURN_NOT_OK(Resize(new_capacity));
+ }
+ UnsafeAppend(data, length);
+ return Status::OK();
+ }
+
+ /// \brief Append the given data to the buffer
+ ///
+ /// The buffer is automatically expanded if necessary.
+ template <size_t NBYTES>
+ Status Append(const std::array<uint8_t, NBYTES>& data) {
+ constexpr auto nbytes = static_cast<int64_t>(NBYTES);
+ if (capacity_ < nbytes + size_) {
+ int64_t new_capacity = BitUtil::NextPower2(nbytes + size_);
+ ARROW_RETURN_NOT_OK(Resize(new_capacity));
+ }
+
+ if (nbytes > 0) {
+ std::copy(data.cbegin(), data.cend(), data_ + size_);
+ size_ += nbytes;
+ }
+ return Status::OK();
+ }
+
+ // Advance pointer and zero out memory
+ Status Advance(const int64_t length) {
+ if (capacity_ < length + size_) {
+ int64_t new_capacity = BitUtil::NextPower2(length + size_);
+ ARROW_RETURN_NOT_OK(Resize(new_capacity));
+ }
+ if (length > 0) {
+ memset(data_ + size_, 0, static_cast<size_t>(length));
+ size_ += length;
+ }
+ return Status::OK();
+ }
+
+ // Unsafe methods don't check existing size
+ void UnsafeAppend(const void* data, int64_t length) {
+ if (length > 0) {
+ memcpy(data_ + size_, data, static_cast<size_t>(length));
+ size_ += length;
+ }
+ }
+
+ /// \brief Return result of builder as a Buffer object.
+ ///
+ /// The builder is reset and can be reused afterwards.
+ ///
+ /// \param[out] out the finalized Buffer object
+ /// \param shrink_to_fit if the buffer size is smaller than its capacity,
+ /// reallocate to fit more tightly in memory. Set to false to avoid
+ /// a reallocation, at the expense of potentially more memory consumption.
+ /// \return Status
+ Status Finish(std::shared_ptr<Buffer>* out, bool shrink_to_fit = true) {
+ ARROW_RETURN_NOT_OK(Resize(size_, shrink_to_fit));
+ *out = buffer_;
+ Reset();
+ return Status::OK();
+ }
+
+ void Reset() {
+ buffer_ = NULLPTR;
+ capacity_ = size_ = 0;
+ }
+
+ int64_t capacity() const { return capacity_; }
+ int64_t length() const { return size_; }
+ const uint8_t* data() const { return data_; }
+
+ protected:
+ std::shared_ptr<ResizableBuffer> buffer_;
+ MemoryPool* pool_;
+ uint8_t* data_;
+ int64_t capacity_;
+ int64_t size_;
+};
+
+/// \brief A BufferBuilder subclass with convenience methods to append typed data
+template <typename T>
+class ARROW_EXPORT TypedBufferBuilder : public BufferBuilder {
+ public:
+ explicit TypedBufferBuilder(MemoryPool* pool) : BufferBuilder(pool) {}
+
+ Status Append(T arithmetic_value) {
+ static_assert(std::is_arithmetic<T>::value,
+ "Convenience buffer append only supports arithmetic types");
+ return BufferBuilder::Append(reinterpret_cast<uint8_t*>(&arithmetic_value),
+ sizeof(T));
+ }
+
+ Status Append(const T* arithmetic_values, int64_t num_elements) {
+ static_assert(std::is_arithmetic<T>::value,
+ "Convenience buffer append only supports arithmetic types");
+ return BufferBuilder::Append(reinterpret_cast<const uint8_t*>(arithmetic_values),
+ num_elements * sizeof(T));
+ }
+
+ void UnsafeAppend(T arithmetic_value) {
+ static_assert(std::is_arithmetic<T>::value,
+ "Convenience buffer append only supports arithmetic types");
+ BufferBuilder::UnsafeAppend(reinterpret_cast<uint8_t*>(&arithmetic_value), sizeof(T));
+ }
+
+ void UnsafeAppend(const T* arithmetic_values, int64_t num_elements) {
+ static_assert(std::is_arithmetic<T>::value,
+ "Convenience buffer append only supports arithmetic types");
+ BufferBuilder::UnsafeAppend(reinterpret_cast<const uint8_t*>(arithmetic_values),
+ num_elements * sizeof(T));
+ }
+
+ const T* data() const { return reinterpret_cast<const T*>(data_); }
+ int64_t length() const { return size_ / sizeof(T); }
+ int64_t capacity() const { return capacity_ / sizeof(T); }
+};
+
+} // namespace arrow
+
+#endif // ARROW_BUFFER_BUILDER_H
diff --git a/cpp/src/arrow/buffer-test.cc b/cpp/src/arrow/buffer-test.cc
index 4d16f7f..5d01515 100644
--- a/cpp/src/arrow/buffer-test.cc
+++ b/cpp/src/arrow/buffer-test.cc
@@ -26,6 +26,7 @@
#include <gtest/gtest.h>
+#include "arrow/buffer-builder.h"
#include "arrow/buffer.h"
#include "arrow/memory_pool.h"
#include "arrow/status.h"
diff --git a/cpp/src/arrow/buffer.cc b/cpp/src/arrow/buffer.cc
index 01bb0c3..9534d2b 100644
--- a/cpp/src/arrow/buffer.cc
+++ b/cpp/src/arrow/buffer.cc
@@ -126,25 +126,18 @@ class PoolBuffer : public ResizableBuffer {
}
Status Resize(const int64_t new_size, bool shrink_to_fit = true) override {
- if (!shrink_to_fit || (new_size > size_)) {
- RETURN_NOT_OK(Reserve(new_size));
- } else {
- // Buffer is not growing, so shrink to the requested size without
+ if (mutable_data_ && shrink_to_fit && new_size <= size_) {
+ // Buffer is non-null and is not growing, so shrink to the requested size without
// excess space.
int64_t new_capacity = BitUtil::RoundUpToMultipleOf64(new_size);
if (capacity_ != new_capacity) {
// Buffer hasn't got yet the requested size.
- if (new_size == 0) {
- pool_->Free(mutable_data_, capacity_);
- capacity_ = 0;
- mutable_data_ = nullptr;
- data_ = nullptr;
- } else {
- RETURN_NOT_OK(pool_->Reallocate(capacity_, new_capacity, &mutable_data_));
- data_ = mutable_data_;
- capacity_ = new_capacity;
- }
+ RETURN_NOT_OK(pool_->Reallocate(capacity_, new_capacity, &mutable_data_));
+ data_ = mutable_data_;
+ capacity_ = new_capacity;
}
+ } else {
+ RETURN_NOT_OK(Reserve(new_size));
}
size_ = new_size;
diff --git a/cpp/src/arrow/buffer.h b/cpp/src/arrow/buffer.h
index 6b2ad1b..f004289 100644
--- a/cpp/src/arrow/buffer.h
+++ b/cpp/src/arrow/buffer.h
@@ -19,7 +19,6 @@
#define ARROW_BUFFER_H
#include <algorithm>
-#include <array>
#include <cstdint>
#include <cstring>
#include <memory>
@@ -29,7 +28,6 @@
#include "arrow/memory_pool.h"
#include "arrow/status.h"
-#include "arrow/util/bit-util.h"
#include "arrow/util/macros.h"
#include "arrow/util/visibility.h"
@@ -110,7 +108,10 @@ class ARROW_EXPORT Buffer {
#ifndef NDEBUG
CheckMutable();
#endif
- memset(mutable_data_ + size_, 0, static_cast<size_t>(capacity_ - size_));
+ // A zero-capacity buffer can have a null data pointer
+ if (capacity_ != 0) {
+ memset(mutable_data_ + size_, 0, static_cast<size_t>(capacity_ - size_));
+ }
}
/// \brief Construct a new buffer that owns its memory from a std::string
@@ -391,167 +392,6 @@ Status AllocateEmptyBitmap(int64_t length, std::shared_ptr<Buffer>* out);
/// @}
-// ----------------------------------------------------------------------
-// Buffer builder classes
-
-/// \class BufferBuilder
-/// \brief A class for incrementally building a contiguous chunk of in-memory data
-class ARROW_EXPORT BufferBuilder {
- public:
- explicit BufferBuilder(MemoryPool* pool ARROW_MEMORY_POOL_DEFAULT)
- : pool_(pool), data_(NULLPTR), capacity_(0), size_(0) {}
-
- /// \brief Resize the buffer to the nearest multiple of 64 bytes
- ///
- /// \param elements the new capacity of the of the builder. Will be rounded
- /// up to a multiple of 64 bytes for padding
- /// \param shrink_to_fit if new capacity is smaller than the existing size,
- /// reallocate internal buffer. Set to false to avoid reallocations when
- /// shrinking the builder.
- /// \return Status
- Status Resize(const int64_t elements, bool shrink_to_fit = true) {
- // Resize(0) is a no-op
- if (elements == 0) {
- return Status::OK();
- }
- int64_t old_capacity = capacity_;
-
- if (buffer_ == NULLPTR) {
- ARROW_RETURN_NOT_OK(AllocateResizableBuffer(pool_, elements, &buffer_));
- } else {
- ARROW_RETURN_NOT_OK(buffer_->Resize(elements, shrink_to_fit));
- }
- capacity_ = buffer_->capacity();
- data_ = buffer_->mutable_data();
- if (capacity_ > old_capacity) {
- memset(data_ + old_capacity, 0, capacity_ - old_capacity);
- }
- return Status::OK();
- }
-
- /// \brief Ensure that builder can accommodate the additional number of bytes
- /// without the need to perform allocations
- ///
- /// \param size number of additional bytes to make space for
- /// \return Status
- Status Reserve(const int64_t size) { return Resize(size_ + size, false); }
-
- /// \brief Append the given data to the buffer
- ///
- /// The buffer is automatically expanded if necessary.
- Status Append(const void* data, int64_t length) {
- if (capacity_ < length + size_) {
- int64_t new_capacity = BitUtil::NextPower2(length + size_);
- ARROW_RETURN_NOT_OK(Resize(new_capacity));
- }
- UnsafeAppend(data, length);
- return Status::OK();
- }
-
- /// \brief Append the given data to the buffer
- ///
- /// The buffer is automatically expanded if necessary.
- template <size_t NBYTES>
- Status Append(const std::array<uint8_t, NBYTES>& data) {
- constexpr auto nbytes = static_cast<int64_t>(NBYTES);
- if (capacity_ < nbytes + size_) {
- int64_t new_capacity = BitUtil::NextPower2(nbytes + size_);
- ARROW_RETURN_NOT_OK(Resize(new_capacity));
- }
-
- std::copy(data.cbegin(), data.cend(), data_ + size_);
- size_ += nbytes;
- return Status::OK();
- }
-
- // Advance pointer and zero out memory
- Status Advance(const int64_t length) {
- if (capacity_ < length + size_) {
- int64_t new_capacity = BitUtil::NextPower2(length + size_);
- ARROW_RETURN_NOT_OK(Resize(new_capacity));
- }
- memset(data_ + size_, 0, static_cast<size_t>(length));
- size_ += length;
- return Status::OK();
- }
-
- // Unsafe methods don't check existing size
- void UnsafeAppend(const void* data, int64_t length) {
- memcpy(data_ + size_, data, static_cast<size_t>(length));
- size_ += length;
- }
-
- /// \brief Return result of builder as a Buffer object.
- ///
- /// The builder is reset and can be reused afterwards.
- ///
- /// \param[out] out the finalized Buffer object
- /// \param shrink_to_fit if the buffer size is smaller than its capacity,
- /// reallocate to fit more tightly in memory. Set to false to avoid
- /// a reallocation, at the expense of potentially more memory consumption.
- /// \return Status
- Status Finish(std::shared_ptr<Buffer>* out, bool shrink_to_fit = true) {
- ARROW_RETURN_NOT_OK(Resize(size_, shrink_to_fit));
- *out = buffer_;
- Reset();
- return Status::OK();
- }
-
- void Reset() {
- buffer_ = NULLPTR;
- capacity_ = size_ = 0;
- }
-
- int64_t capacity() const { return capacity_; }
- int64_t length() const { return size_; }
- const uint8_t* data() const { return data_; }
-
- protected:
- std::shared_ptr<ResizableBuffer> buffer_;
- MemoryPool* pool_;
- uint8_t* data_;
- int64_t capacity_;
- int64_t size_;
-};
-
-/// \brief A BufferBuilder subclass with convenience methods to append typed data
-template <typename T>
-class ARROW_EXPORT TypedBufferBuilder : public BufferBuilder {
- public:
- explicit TypedBufferBuilder(MemoryPool* pool) : BufferBuilder(pool) {}
-
- Status Append(T arithmetic_value) {
- static_assert(std::is_arithmetic<T>::value,
- "Convenience buffer append only supports arithmetic types");
- return BufferBuilder::Append(reinterpret_cast<uint8_t*>(&arithmetic_value),
- sizeof(T));
- }
-
- Status Append(const T* arithmetic_values, int64_t num_elements) {
- static_assert(std::is_arithmetic<T>::value,
- "Convenience buffer append only supports arithmetic types");
- return BufferBuilder::Append(reinterpret_cast<const uint8_t*>(arithmetic_values),
- num_elements * sizeof(T));
- }
-
- void UnsafeAppend(T arithmetic_value) {
- static_assert(std::is_arithmetic<T>::value,
- "Convenience buffer append only supports arithmetic types");
- BufferBuilder::UnsafeAppend(reinterpret_cast<uint8_t*>(&arithmetic_value), sizeof(T));
- }
-
- void UnsafeAppend(const T* arithmetic_values, int64_t num_elements) {
- static_assert(std::is_arithmetic<T>::value,
- "Convenience buffer append only supports arithmetic types");
- BufferBuilder::UnsafeAppend(reinterpret_cast<const uint8_t*>(arithmetic_values),
- num_elements * sizeof(T));
- }
-
- const T* data() const { return reinterpret_cast<const T*>(data_); }
- int64_t length() const { return size_ / sizeof(T); }
- int64_t capacity() const { return capacity_ / sizeof(T); }
-};
-
} // namespace arrow
#endif // ARROW_BUFFER_H
diff --git a/cpp/src/arrow/io/readahead.cc b/cpp/src/arrow/io/readahead.cc
index 89db6a6..4222f87 100644
--- a/cpp/src/arrow/io/readahead.cc
+++ b/cpp/src/arrow/io/readahead.cc
@@ -162,11 +162,13 @@ class ReadaheadSpooler::Impl {
int64_t bytes_read;
RETURN_NOT_OK(AllocateResizableBuffer(
pool_, read_size_ + buf->left_padding + buf->right_padding, &buffer));
+ DCHECK_NE(buffer->mutable_data(), nullptr);
RETURN_NOT_OK(
raw_->Read(read_size_, &bytes_read, buffer->mutable_data() + buf->left_padding));
if (bytes_read < read_size_) {
// Got a short read
RETURN_NOT_OK(buffer->Resize(bytes_read + buf->left_padding + buf->right_padding));
+ DCHECK_NE(buffer->mutable_data(), nullptr);
}
// Zero padding areas
memset(buffer->mutable_data(), 0, buf->left_padding);
diff --git a/cpp/src/arrow/memory_pool.cc b/cpp/src/arrow/memory_pool.cc
index fb5beac..abf36fc 100644
--- a/cpp/src/arrow/memory_pool.cc
+++ b/cpp/src/arrow/memory_pool.cc
@@ -40,6 +40,14 @@ namespace arrow {
constexpr size_t kAlignment = 64;
namespace {
+
+#ifdef ARROW_JEMALLOC
+inline size_t FixAllocationSize(int64_t size) {
+ // mallocx() and rallocx() don't support 0-sized allocations
+ return std::max(static_cast<size_t>(size), kAlignment);
+}
+#endif
+
// Allocate memory according to the alignment requirements for Arrow
// (as of May 2016 64 bytes)
Status AllocateAligned(int64_t size, uint8_t** out) {
@@ -58,8 +66,8 @@ Status AllocateAligned(int64_t size, uint8_t** out) {
return Status::OutOfMemory("malloc of size ", size, " failed");
}
#elif defined(ARROW_JEMALLOC)
- *out = reinterpret_cast<uint8_t*>(mallocx(
- std::max(static_cast<size_t>(size), kAlignment), MALLOCX_ALIGN(kAlignment)));
+ *out = reinterpret_cast<uint8_t*>(
+ mallocx(FixAllocationSize(size), MALLOCX_ALIGN(kAlignment)));
if (*out == NULL) {
return Status::OutOfMemory("malloc of size ", size, " failed");
}
@@ -76,6 +84,42 @@ Status AllocateAligned(int64_t size, uint8_t** out) {
#endif
return Status::OK();
}
+
+Status ReallocateAligned(int64_t old_size, int64_t new_size, uint8_t** ptr) {
+#ifdef ARROW_JEMALLOC
+ uint8_t* previous_ptr = *ptr;
+ if (new_size < 0) {
+ return Status::Invalid("negative realloc size");
+ }
+ if (static_cast<uint64_t>(new_size) >= std::numeric_limits<size_t>::max()) {
+ return Status::CapacityError("realloc overflows size_t");
+ }
+ *ptr = reinterpret_cast<uint8_t*>(
+ rallocx(*ptr, FixAllocationSize(new_size), MALLOCX_ALIGN(kAlignment)));
+ if (*ptr == NULL) {
+ *ptr = previous_ptr;
+ return Status::OutOfMemory("realloc of size ", new_size, " failed");
+ }
+#else
+ // Note: We cannot use realloc() here as it doesn't guarantee alignment.
+
+ // Allocate new chunk
+ uint8_t* out = nullptr;
+ RETURN_NOT_OK(AllocateAligned(new_size, &out));
+ DCHECK(out);
+ // Copy contents and release old memory chunk
+ memcpy(out, *ptr, static_cast<size_t>(std::min(new_size, old_size)));
+#ifdef _WIN32
+ _aligned_free(*ptr);
+#else
+ std::free(*ptr);
+#endif // defined(_MSC_VER)
+ *ptr = out;
+#endif // defined(ARROW_JEMALLOC)
+
+ return Status::OK();
+}
+
} // namespace
MemoryPool::MemoryPool() {}
@@ -99,36 +143,7 @@ class DefaultMemoryPool : public MemoryPool {
}
Status Reallocate(int64_t old_size, int64_t new_size, uint8_t** ptr) override {
-#ifdef ARROW_JEMALLOC
- uint8_t* previous_ptr = *ptr;
- if (new_size < 0) {
- return Status::Invalid("negative realloc size");
- }
- if (static_cast<uint64_t>(new_size) >= std::numeric_limits<size_t>::max()) {
- return Status::CapacityError("realloc overflows size_t");
- }
- *ptr = reinterpret_cast<uint8_t*>(
- rallocx(*ptr, static_cast<size_t>(new_size), MALLOCX_ALIGN(kAlignment)));
- if (*ptr == NULL) {
- *ptr = previous_ptr;
- return Status::OutOfMemory("realloc of size ", new_size, " failed");
- }
-#else
- // Note: We cannot use realloc() here as it doesn't guarantee alignment.
-
- // Allocate new chunk
- uint8_t* out = nullptr;
- RETURN_NOT_OK(AllocateAligned(new_size, &out));
- DCHECK(out);
- // Copy contents and release old memory chunk
- memcpy(out, *ptr, static_cast<size_t>(std::min(new_size, old_size)));
-#ifdef _WIN32
- _aligned_free(*ptr);
-#else
- std::free(*ptr);
-#endif // defined(_MSC_VER)
- *ptr = out;
-#endif // defined(ARROW_JEMALLOC)
+ RETURN_NOT_OK(ReallocateAligned(old_size, new_size, ptr));
stats_.UpdateAllocatedBytes(new_size - old_size);
return Status::OK();
diff --git a/cpp/src/arrow/test-util.cc b/cpp/src/arrow/test-util.cc
index 8c5f364..617c539 100644
--- a/cpp/src/arrow/test-util.cc
+++ b/cpp/src/arrow/test-util.cc
@@ -303,17 +303,23 @@ void AssertZeroPadded(const Array& array) {
for (const auto& buffer : array.data()->buffers) {
if (buffer) {
const int64_t padding = buffer->capacity() - buffer->size();
- std::vector<uint8_t> zeros(padding);
- ASSERT_EQ(0, memcmp(buffer->data() + buffer->size(), zeros.data(), padding));
+ if (padding > 0) {
+ std::vector<uint8_t> zeros(padding);
+ ASSERT_EQ(0, memcmp(buffer->data() + buffer->size(), zeros.data(), padding));
+ }
}
}
}
void TestInitialized(const Array& array) {
for (const auto& buffer : array.data()->buffers) {
- if (buffer) {
- std::vector<uint8_t> zeros(buffer->capacity());
- throw_away = memcmp(buffer->data(), zeros.data(), buffer->size());
+ if (buffer && buffer->capacity() > 0) {
+ int total = 0;
+ auto data = buffer->data();
+ for (int64_t i = 0; i < buffer->size(); ++i) {
+ total ^= data[i];
+ }
+ throw_away = total;
}
}
}
diff --git a/cpp/src/arrow/util/thread-pool-test.cc b/cpp/src/arrow/util/thread-pool-test.cc
index 6d7b9e2..22a8db2 100644
--- a/cpp/src/arrow/util/thread-pool-test.cc
+++ b/cpp/src/arrow/util/thread-pool-test.cc
@@ -298,7 +298,7 @@ TEST_F(TestThreadPool, Submit) {
// Test fork safety on Unix
-#if !(defined(_WIN32) || defined(ARROW_VALGRIND))
+#if !(defined(_WIN32) || defined(ARROW_VALGRIND) || defined(ADDRESS_SANITIZER))
TEST_F(TestThreadPool, ForkSafety) {
pid_t child_pid;
int child_status;
diff --git a/cpp/src/parquet/bloom_filter.h b/cpp/src/parquet/bloom_filter.h
index 918780e..0078051 100644
--- a/cpp/src/parquet/bloom_filter.h
+++ b/cpp/src/parquet/bloom_filter.h
@@ -21,6 +21,7 @@
#include <cstdint>
#include <memory>
+#include "arrow/util/bit-util.h"
#include "arrow/util/logging.h"
#include "parquet/exception.h"
#include "parquet/hasher.h"