From 2f7087eba15d3693d3557f1e21b8819cc2c760b2 Mon Sep 17 00:00:00 2001 From: sriramch <33358417+sriramch@users.noreply.github.com> Date: Sun, 31 Mar 2019 07:48:58 -0700 Subject: [PATCH] Improve HostDeviceVector exception safety (#4301) * make the assignments of HostDeviceVector exception safe. * storing a dummy GPUDistribution instance in HDV for CPU based code. * change testxgboost binary location to build directory. --- CMakeLists.txt | 2 +- doc/contribute.rst | 2 +- src/common/host_device_vector.cc | 30 +++++++++++++++++------------- src/common/host_device_vector.cu | 14 ++++++++------ src/common/host_device_vector.h | 10 +++++----- tests/ci_build/test_gpu.sh | 2 +- tests/ci_build/test_mgpu.sh | 2 +- tests/travis/run_test.sh | 4 ++-- 8 files changed, 36 insertions(+), 30 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 19ba17612..cfd526948 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -340,7 +340,7 @@ if(GOOGLE_TEST) $) endif () - set_output_directory(testxgboost ${PROJECT_SOURCE_DIR}) + set_output_directory(testxgboost ${CMAKE_BINARY_DIR}) target_include_directories(testxgboost PRIVATE ${GTEST_INCLUDE_DIRS}) target_link_libraries(testxgboost ${GTEST_LIBRARIES} ${LINK_LIBRARIES}) diff --git a/doc/contribute.rst b/doc/contribute.rst index cde56efe0..09324e20a 100644 --- a/doc/contribute.rst +++ b/doc/contribute.rst @@ -166,7 +166,7 @@ environment variable: .. code-block:: bash - ASAN_OPTIONS=protect_shadow_gap=0 ../testxgboost + ASAN_OPTIONS=protect_shadow_gap=0 ${BUILD_DIR}/testxgboost For details, please consult `official documentation `_ for sanitizers. diff --git a/src/common/host_device_vector.cc b/src/common/host_device_vector.cc index bb8450526..38d3a3c27 100644 --- a/src/common/host_device_vector.cc +++ b/src/common/host_device_vector.cc @@ -15,41 +15,42 @@ namespace xgboost { template struct HostDeviceVectorImpl { - explicit HostDeviceVectorImpl(size_t size, T v) : data_h_(size, v), distribution_() {} - HostDeviceVectorImpl(std::initializer_list init) : data_h_(init), distribution_() {} - explicit HostDeviceVectorImpl(std::vector init) : data_h_(std::move(init)), distribution_() {} + explicit HostDeviceVectorImpl(size_t size, T v) : data_h_(size, v) {} + HostDeviceVectorImpl(std::initializer_list init) : data_h_(init) {} + explicit HostDeviceVectorImpl(std::vector init) : data_h_(std::move(init)) {} + + void Swap(HostDeviceVectorImpl &other) { + data_h_.swap(other.data_h_); + } std::vector& Vec() { return data_h_; } - GPUDistribution& Dist() { return distribution_; } private: std::vector data_h_; - GPUDistribution distribution_; }; template -HostDeviceVector::HostDeviceVector(size_t size, T v, GPUDistribution distribution) +HostDeviceVector::HostDeviceVector(size_t size, T v, const GPUDistribution &) : impl_(nullptr) { impl_ = new HostDeviceVectorImpl(size, v); } template -HostDeviceVector::HostDeviceVector(std::initializer_list init, GPUDistribution distribution) +HostDeviceVector::HostDeviceVector(std::initializer_list init, const GPUDistribution &) : impl_(nullptr) { impl_ = new HostDeviceVectorImpl(init); } template -HostDeviceVector::HostDeviceVector(const std::vector& init, GPUDistribution distribution) +HostDeviceVector::HostDeviceVector(const std::vector& init, const GPUDistribution &) : impl_(nullptr) { impl_ = new HostDeviceVectorImpl(init); } template HostDeviceVector::~HostDeviceVector() { - HostDeviceVectorImpl* tmp = impl_; + delete impl_; impl_ = nullptr; - delete tmp; } template @@ -63,8 +64,10 @@ HostDeviceVector& HostDeviceVector::operator=(const HostDeviceVector& o if (this == &other) { return *this; } - delete impl_; - impl_ = new HostDeviceVectorImpl(*other.impl_); + + HostDeviceVectorImpl newInstance(*other.impl_); + newInstance.Swap(*impl_); + return *this; } @@ -76,7 +79,8 @@ GPUSet HostDeviceVector::Devices() const { return GPUSet::Empty(); } template const GPUDistribution& HostDeviceVector::Distribution() const { - return impl_->Dist(); + static GPUDistribution dummyInstance; + return dummyInstance; } template diff --git a/src/common/host_device_vector.cu b/src/common/host_device_vector.cu index 55450f3bd..5e7634501 100644 --- a/src/common/host_device_vector.cu +++ b/src/common/host_device_vector.cu @@ -171,7 +171,7 @@ struct HostDeviceVectorImpl { HostDeviceVectorImpl* vec_; }; - HostDeviceVectorImpl(size_t size, T v, GPUDistribution distribution) + HostDeviceVectorImpl(size_t size, T v, const GPUDistribution &distribution) : distribution_(distribution), perm_h_(distribution.IsEmpty()), size_d_(0) { if (!distribution_.IsEmpty()) { size_d_ = size; @@ -194,7 +194,7 @@ struct HostDeviceVectorImpl { // Initializer can be std::vector or std::initializer_list template - HostDeviceVectorImpl(const Initializer& init, GPUDistribution distribution) + HostDeviceVectorImpl(const Initializer& init, const GPUDistribution &distribution) : distribution_(distribution), perm_h_(distribution.IsEmpty()), size_d_(0) { if (!distribution_.IsEmpty()) { size_d_ = init.size(); @@ -435,19 +435,19 @@ struct HostDeviceVectorImpl { template HostDeviceVector::HostDeviceVector -(size_t size, T v, GPUDistribution distribution) : impl_(nullptr) { +(size_t size, T v, const GPUDistribution &distribution) : impl_(nullptr) { impl_ = new HostDeviceVectorImpl(size, v, distribution); } template HostDeviceVector::HostDeviceVector -(std::initializer_list init, GPUDistribution distribution) : impl_(nullptr) { +(std::initializer_list init, const GPUDistribution &distribution) : impl_(nullptr) { impl_ = new HostDeviceVectorImpl(init, distribution); } template HostDeviceVector::HostDeviceVector -(const std::vector& init, GPUDistribution distribution) : impl_(nullptr) { +(const std::vector& init, const GPUDistribution &distribution) : impl_(nullptr) { impl_ = new HostDeviceVectorImpl(init, distribution); } @@ -461,8 +461,10 @@ template HostDeviceVector& HostDeviceVector::operator= (const HostDeviceVector& other) { if (this == &other) { return *this; } + + std::unique_ptr> newImpl(new HostDeviceVectorImpl(*other.impl_)); delete impl_; - impl_ = new HostDeviceVectorImpl(*other.impl_); + impl_ = newImpl.release(); return *this; } diff --git a/src/common/host_device_vector.h b/src/common/host_device_vector.h index b1ae87e63..425cbff53 100644 --- a/src/common/host_device_vector.h +++ b/src/common/host_device_vector.h @@ -93,7 +93,7 @@ class GPUDistribution { private: GPUDistribution(GPUSet devices, int granularity, int overlap, - std::vector offsets) + std::vector &&offsets) : devices_(devices), granularity_(granularity), overlap_(overlap), offsets_(std::move(offsets)) {} @@ -109,7 +109,7 @@ class GPUDistribution { } static GPUDistribution Explicit(GPUSet devices, std::vector offsets) { - return GPUDistribution(devices, 1, 0, offsets); + return GPUDistribution(devices, 1, 0, std::move(offsets)); } friend bool operator==(const GPUDistribution& a, const GPUDistribution& b) { @@ -195,11 +195,11 @@ template class HostDeviceVector { public: explicit HostDeviceVector(size_t size = 0, T v = T(), - GPUDistribution distribution = GPUDistribution()); + const GPUDistribution &distribution = GPUDistribution()); HostDeviceVector(std::initializer_list init, - GPUDistribution distribution = GPUDistribution()); + const GPUDistribution &distribution = GPUDistribution()); explicit HostDeviceVector(const std::vector& init, - GPUDistribution distribution = GPUDistribution()); + const GPUDistribution &distribution = GPUDistribution()); ~HostDeviceVector(); HostDeviceVector(const HostDeviceVector&); HostDeviceVector& operator=(const HostDeviceVector&); diff --git a/tests/ci_build/test_gpu.sh b/tests/ci_build/test_gpu.sh index adb275481..d82485b22 100755 --- a/tests/ci_build/test_gpu.sh +++ b/tests/ci_build/test_gpu.sh @@ -5,4 +5,4 @@ cd python-package python setup.py install --user cd .. pytest -v -s --fulltrace -m "(not mgpu) and (not slow)" tests/python-gpu -./testxgboost --gtest_filter=-*.MGPU_* +./build/testxgboost --gtest_filter=-*.MGPU_* diff --git a/tests/ci_build/test_mgpu.sh b/tests/ci_build/test_mgpu.sh index a1b56549b..b003d9c42 100755 --- a/tests/ci_build/test_mgpu.sh +++ b/tests/ci_build/test_mgpu.sh @@ -5,7 +5,7 @@ cd python-package python setup.py install --user cd .. pytest -v -s --fulltrace -m "(not slow) and mgpu" tests/python-gpu -./testxgboost --gtest_filter=*.MGPU_* +./build/testxgboost --gtest_filter=*.MGPU_* cd tests/distributed ./runtests-gpu.sh diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index 962237443..773452cd4 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -123,8 +123,8 @@ if [ ${TASK} == "cmake_test" ]; then PLUGINS="-DPLUGIN_LZ4=ON -DPLUGIN_DENSE_PARSER=ON" cmake .. -DGOOGLE_TEST=ON -DGTEST_ROOT=$PWD/../gtest/ ${PLUGINS} make - cd .. ./testxgboost + cd .. rm -rf build fi @@ -170,10 +170,10 @@ if [ ${TASK} == "sanitizer_test" ]; then -DCMAKE_CXX_FLAGS="-fuse-ld=gold" \ -DCMAKE_C_FLAGS="-fuse-ld=gold" make - cd .. export ASAN_SYMBOLIZER_PATH=$(which llvm-symbolizer) ASAN_OPTIONS=symbolize=1 ./testxgboost + cd .. rm -rf build exit 0 fi