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

@@ -340,7 +340,7 @@ if(GOOGLE_TEST)
       $<TARGET_OBJECTS:objxgboost>)
   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})

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 <https://github.com/google/sanitizers/wiki>`_ for sanitizers.
 

src/common/host_device_vector.cc

@@ -15,41 +15,42 @@ namespace xgboost {
 
 template <typename T>
 struct HostDeviceVectorImpl {
-  explicit HostDeviceVectorImpl(size_t size, T v) : data_h_(size, v), distribution_() {}
+  explicit HostDeviceVectorImpl(size_t size, T v) : data_h_(size, v) {}
-  HostDeviceVectorImpl(std::initializer_list<T> init) : data_h_(init), distribution_() {}
+  HostDeviceVectorImpl(std::initializer_list<T> init) : data_h_(init) {}
-  explicit HostDeviceVectorImpl(std::vector<T>  init) : data_h_(std::move(init)), distribution_() {}
+  explicit HostDeviceVectorImpl(std::vector<T>  init) : data_h_(std::move(init)) {}
+
+  void Swap(HostDeviceVectorImpl &other) {
+     data_h_.swap(other.data_h_);
+  }
 
   std::vector<T>& Vec() { return data_h_; }
-  GPUDistribution& Dist() { return distribution_; }
 
  private:
   std::vector<T> data_h_;
-  GPUDistribution distribution_;
 };
 
 template <typename T>
-HostDeviceVector<T>::HostDeviceVector(size_t size, T v, GPUDistribution distribution)
+HostDeviceVector<T>::HostDeviceVector(size_t size, T v, const GPUDistribution &)
   : impl_(nullptr) {
   impl_ = new HostDeviceVectorImpl<T>(size, v);
 }
 
 template <typename T>
-HostDeviceVector<T>::HostDeviceVector(std::initializer_list<T> init, GPUDistribution distribution)
+HostDeviceVector<T>::HostDeviceVector(std::initializer_list<T> init, const GPUDistribution &)
   : impl_(nullptr) {
   impl_ = new HostDeviceVectorImpl<T>(init);
 }
 
 template <typename T>
-HostDeviceVector<T>::HostDeviceVector(const std::vector<T>& init, GPUDistribution distribution)
+HostDeviceVector<T>::HostDeviceVector(const std::vector<T>& init, const GPUDistribution &)
   : impl_(nullptr) {
   impl_ = new HostDeviceVectorImpl<T>(init);
 }
 
 template <typename T>
 HostDeviceVector<T>::~HostDeviceVector() {
-  HostDeviceVectorImpl<T>* tmp = impl_;
+  delete impl_;
   impl_ = nullptr;
-  delete tmp;
 }
 
 template <typename T>
@@ -63,8 +64,10 @@ HostDeviceVector<T>& HostDeviceVector<T>::operator=(const HostDeviceVector<T>& o
   if (this == &other) {
     return *this;
   }
-  delete impl_;
+
-  impl_ = new HostDeviceVectorImpl<T>(*other.impl_);
+  HostDeviceVectorImpl<T> newInstance(*other.impl_);
+  newInstance.Swap(*impl_);
+
   return *this;
 }
 
@@ -76,7 +79,8 @@ GPUSet HostDeviceVector<T>::Devices() const { return GPUSet::Empty(); }
 
 template <typename T>
 const GPUDistribution& HostDeviceVector<T>::Distribution() const {
-  return impl_->Dist();
+  static GPUDistribution dummyInstance;
+  return dummyInstance;
 }
 
 template <typename T>

src/common/host_device_vector.cu

@@ -171,7 +171,7 @@ struct HostDeviceVectorImpl {
     HostDeviceVectorImpl<T>* 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<T> or std::initializer_list<T>
   template <class Initializer>
-  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 <typename T>
 HostDeviceVector<T>::HostDeviceVector
-(size_t size, T v, GPUDistribution distribution) : impl_(nullptr) {
+(size_t size, T v, const GPUDistribution &distribution) : impl_(nullptr) {
   impl_ = new HostDeviceVectorImpl<T>(size, v, distribution);
 }
 
 template <typename T>
 HostDeviceVector<T>::HostDeviceVector
-(std::initializer_list<T> init, GPUDistribution distribution) : impl_(nullptr) {
+(std::initializer_list<T> init, const GPUDistribution &distribution) : impl_(nullptr) {
   impl_ = new HostDeviceVectorImpl<T>(init, distribution);
 }
 
 template <typename T>
 HostDeviceVector<T>::HostDeviceVector
-(const std::vector<T>& init, GPUDistribution distribution) : impl_(nullptr) {
+(const std::vector<T>& init, const GPUDistribution &distribution) : impl_(nullptr) {
   impl_ = new HostDeviceVectorImpl<T>(init, distribution);
 }
 
@@ -461,8 +461,10 @@ template <typename T>
 HostDeviceVector<T>& HostDeviceVector<T>::operator=
 (const HostDeviceVector<T>& other) {
   if (this == &other) { return *this; }
+
+  std::unique_ptr<HostDeviceVectorImpl<T>> newImpl(new HostDeviceVectorImpl<T>(*other.impl_));
   delete impl_;
-  impl_ = new HostDeviceVectorImpl<T>(*other.impl_);
+  impl_ = newImpl.release();
   return *this;
 }
 

src/common/host_device_vector.h

@@ -93,7 +93,7 @@ class GPUDistribution {
 
  private:
   GPUDistribution(GPUSet devices, int granularity, int overlap,
-                  std::vector<size_t> offsets)
+                  std::vector<size_t> &&offsets)
     : devices_(devices), granularity_(granularity), overlap_(overlap),
     offsets_(std::move(offsets)) {}
 
@@ -109,7 +109,7 @@ class GPUDistribution {
   }
 
   static GPUDistribution Explicit(GPUSet devices, std::vector<size_t> 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 <typename T>
 class HostDeviceVector {
  public:
   explicit HostDeviceVector(size_t size = 0, T v = T(),
-                            GPUDistribution distribution = GPUDistribution());
+                            const GPUDistribution &distribution = GPUDistribution());
   HostDeviceVector(std::initializer_list<T> init,
-                   GPUDistribution distribution = GPUDistribution());
+                   const GPUDistribution &distribution = GPUDistribution());
   explicit HostDeviceVector(const std::vector<T>& init,
-                            GPUDistribution distribution = GPUDistribution());
+                            const GPUDistribution &distribution = GPUDistribution());
   ~HostDeviceVector();
   HostDeviceVector(const HostDeviceVector<T>&);
   HostDeviceVector<T>& operator=(const HostDeviceVector<T>&);

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_*

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

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