Add NVTX op tracing for Reducescatter, make some base class destructo…

…rs virtual (#3574) Signed-off-by: Max H. Gerlach <git@maxgerlach.de>
horovod · Jun 20, 2022 · 176251b · 176251b
1 parent 21adc81
commit 176251b
Show file tree

Hide file tree

Showing 7 changed files with 9 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Added
 
-- Added `hvd.reducescatter()` operation with implementations in NCCL, MPI, and Gloo. ([#3299](https://github.com/horovod/horovod/pull/3299))
+- Added `hvd.reducescatter()` operation with implementations in NCCL, MPI, and Gloo. ([#3299](https://github.com/horovod/horovod/pull/3299), [#3574](https://github.com/horovod/horovod/pull/3574))
 
 - Spark: expose random seed as an optional parameter. ([#3517](https://github.com/horovod/horovod/pull/3517))
 

diff --git a/horovod/common/controller.h b/horovod/common/controller.h
@@ -42,6 +42,8 @@ class Controller : public std::enable_shared_from_this<Controller> {
 
   Controller(const Controller&) = delete;
 
+  virtual ~Controller() = default;
+
   void Initialize();
 
   virtual int GetTypeSize(DataType dtype) = 0;

diff --git a/horovod/common/mpi/mpi_controller.h b/horovod/common/mpi/mpi_controller.h
@@ -36,8 +36,6 @@ class MPIController : public Controller {
     LOG(DEBUG) << "MPI Controller constructed.";
   }
 
-  virtual ~MPIController()=default;
-
   int GetTypeSize(DataType dtype) override;
 
   void CrossRankBitwiseAnd(std::vector<long long>& bitvector,

diff --git a/horovod/common/nvtx_op_range.cc b/horovod/common/nvtx_op_range.cc
@@ -14,6 +14,7 @@ NvtxOpsHandle::NvtxOpsHandle() noexcept
   REGISTER_STRING(HorovodAllgather);
   REGISTER_STRING(HorovodBroadcast);
   REGISTER_STRING(HorovodAlltoall);
+  REGISTER_STRING(HorovodReducescatter);
 #undef REGISTER_STRING
 }
 

diff --git a/horovod/common/nvtx_op_range.h b/horovod/common/nvtx_op_range.h
@@ -16,7 +16,8 @@ enum class RegisteredNvtxOp {
   HorovodBroadcast,
   HorovodAlltoall,
   HorovodReducescatter,
-  // Insert new enum values above this line
+  // Insert values for new ops above this line. Also add corresponding
+  // REGISTER_STRING lines in the constructor NvtxOpsHandle::NvtxOpsHandle().
   END,
 };
 

diff --git a/horovod/common/operations.cc b/horovod/common/operations.cc
@@ -629,7 +629,7 @@ void BackgroundThreadLoop(HorovodGlobalState& state) {
       int id;
       GetProcessSetOrAddUnitialized(process_set_ranks, id);
     }
-    int32_t initialized_count;
+    int32_t initialized_count = 0;
 #if HAVE_MPI
     if (state.control_operation == LibType::MPI) {
       initialized_count =

diff --git a/horovod/common/ops/gloo_operations.h b/horovod/common/ops/gloo_operations.h
@@ -43,14 +43,14 @@ class IGlooAlgorithms {
                              std::vector<int>& recvcounts) = 0;
 
   virtual int ElementSize() const = 0;
+
+  virtual ~IGlooAlgorithms() = default;
 };
 
 template <typename T> class GlooAlgorithms : public IGlooAlgorithms {
 public:
   explicit GlooAlgorithms(GlooContext* gloo_context);
 
-  ~GlooAlgorithms() = default;
-
   void Allreduce(void* buffer_data, int num_elements) override;
 
   void Allgather(void* buffer_data, void* buffer_out, int* recvcounts,
@@ -74,8 +74,6 @@ class GlooAllreduce : public AllreduceOp {
 public:
   explicit GlooAllreduce(HorovodGlobalState* global_state);
 
-  virtual ~GlooAllreduce() = default;
-
   Status Execute(std::vector<TensorTableEntry>& entries,
                  const Response& response) override;