piiswrong commented on a change in pull request #8915: NVLink communication 
pattern updated 
URL: https://github.com/apache/incubator-mxnet/pull/8915#discussion_r170374876
 
 

 ##########
 File path: src/kvstore/comm.h
 ##########
 @@ -178,85 +179,94 @@ class CommCPU : public Comm {
         const_vars[i] = reduce[i].var();
       }
       NDArray result = buf.merged;
-      Resource rsc = ResourceManager::Get()->Request(result.ctx(),
-          ResourceRequest(ResourceRequest::kTempSpace));
+      Resource rsc = ResourceManager::Get()->Request(
+          result.ctx(), ResourceRequest(ResourceRequest::kTempSpace));
       Engine::Get()->PushAsync(
-        [reduce, result, rsc, this](RunContext rctx, 
Engine::CallbackOnComplete on_complete) {
-          NDArray out = result;
-          is_serial_push_?
-            ReduceSumCPUExSerial(reduce, &out)
-            : mxnet::ndarray::ElementwiseSum(rctx.get_stream<cpu>(), rsc, 
reduce, &out);
-          on_complete();
-        }, Context::CPU(), const_vars, {result.var(), rsc.var},
-        FnProperty::kCPUPrioritized, priority, 
PROFILER_MESSAGE("KVStoreReduce"));
+          [reduce, result, rsc, this](RunContext rctx,
+                                      Engine::CallbackOnComplete on_complete) {
+            NDArray out = result;
+            is_serial_push_ ? ReduceSumCPUExSerial(reduce, &out)
+                            : mxnet::ndarray::ElementwiseSum(
+                                  rctx.get_stream<cpu>(), rsc, reduce, &out);
+            on_complete();
+          },
+          Context::CPU(), const_vars, {result.var(), rsc.var},
+          FnProperty::kCPUPrioritized, priority,
+          PROFILER_MESSAGE("KVStoreReduce"));
     }
 
     return buf.merged;
   }
 
-  void Broadcast(int key, const NDArray& src,
-                 const std::vector<NDArray*> dst, int priority) override {
+  void Broadcast(int key, const NDArray& src, const std::vector<NDArray*> dst,
+                 int priority) override {
     int mask = src.ctx().dev_mask();
     if (mask == Context::kCPU) {
-      for (auto d : dst) CopyFromTo(src, d, priority);
+      for (auto& d : dst) CopyFromTo(src, d, priority);
     } else {
       // first copy data to cpu, then broadcast
-      auto& buf = merge_buf_[key];
+      BufferEntry& buf = merge_buf_[key];
       CopyFromTo(src, &buf.merged, priority);
-      for (auto d : dst) CopyFromTo(buf.merged, d, priority);
+      for (auto& d : dst) CopyFromTo(buf.merged, d, priority);
     }
   }
 
   void BroadcastRowSparse(int key, const NDArray& src,
                           const std::vector<std::pair<NDArray*, NDArray>>& dst,
-                          const bool use_copy,
-                          const int priority) override {
+                          const bool use_copy, const int priority) override {
     using namespace mshadow;
     CHECK_EQ(src.storage_type(), kRowSparseStorage)
-      << "BroadcastRowSparse expects row-sparse src NDArray";
+        << "BroadcastRowSparse expects row-sparse src NDArray";
     CHECK_EQ(src.ctx().dev_mask(), Context::kCPU)
-      << "BroadcastRowSparse with src on gpu context not supported";
+        << "BroadcastRowSparse with src on gpu context not supported";
     for (size_t i = 0; i < dst.size(); ++i) {
       NDArray* out = dst[i].first;
       NDArray row_id = dst[i].second;
       if (use_copy) {
         CopyFromTo(src, out, priority);
       } else {
         CHECK_EQ(out->storage_type(), kRowSparseStorage)
-                 << "BroadcastRowSparse expects row_sparse dst NDArray";
+            << "BroadcastRowSparse expects row_sparse dst NDArray";
         CHECK_EQ(row_id.ctx().dev_mask(), Context::kCPU)
-                 << "BroadcastRowSparse with row_indices on gpu context not 
supported";
+            << "BroadcastRowSparse with row_indices on gpu context not "
+               "supported";
         // retain according to unique indices
-        const bool use_sparse_retain = (src.shape()[0] != 
src.storage_shape()[0])
-          || (row_id.dtype() != out->aux_type(rowsparse::kIdx))
-          || (out->ctx().dev_mask() != Context::kGPU);
+        const bool use_sparse_retain =
+            (src.shape()[0] != src.storage_shape()[0]) ||
+            (row_id.dtype() != out->aux_type(rowsparse::kIdx)) ||
+            (out->ctx().dev_mask() != Context::kGPU);
         if (use_sparse_retain) {  // use sparse_retain op
           const bool is_to_gpu = out->ctx().dev_mask() == Context::kGPU;
-          NDArray out_cpu = is_to_gpu? NDArray(kRowSparseStorage, src.shape(),
-              src.ctx(), true, src.dtype(), src.aux_types()) : *out;
+          NDArray out_cpu =
+              is_to_gpu ? NDArray(kRowSparseStorage, src.shape(), src.ctx(),
+                                  true, src.dtype(), src.aux_types())
+                        : *out;
           Engine::Get()->PushAsync(
-            [=](RunContext rctx, Engine::CallbackOnComplete on_complete) {
-              const TBlob& indices = row_id.data();
-              NDArray temp = out_cpu;  // get rid of const qualifier
-              op::SparseRetainOpForwardRspImpl<cpu>(rctx.get_stream<cpu>(),
-                                                    src, indices, kWriteTo,
-                                                    &temp);
-              on_complete();
-            }, Context::CPU(), {src.var(), row_id.var()}, {out_cpu.var()},
-            FnProperty::kNormal, priority, 
PROFILER_MESSAGE("KVStoreSparseRetain"));
+              [=](RunContext rctx, Engine::CallbackOnComplete on_complete) {
 
 Review comment:
   this sort of cosmetic changes are really distracting for code review. Try 
not to do it next time

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to