overlap send ops and backward ops #10550

Yancey1989 · 2018-05-10T03:23:48Z

Experiment with vgg16 + flowers on P40, 2 pservers + 2 trainers

branch	GPUs per trainer	executor	performance
develop	1	default executor	12.946086 imgs/s
overlap	1	default executor	14.587641 imgs/s
develop	8	parallel executor	144.8 imgs/s
overlap	8	parallel executor	175 imgs/s

The performance improves 12% on a single device, 20% on multi devices.

…_send_op

typhoonzero · 2018-05-23T12:45:50Z

seems test_dist_transpiler need to be updated

typhoonzero · 2018-05-23T12:49:03Z

python/paddle/fluid/transpiler/ps_dispatcher.py

+
+class PSDispatcher(object):
+    """
+    DistributedSpliter is the base class for dispatching vars


DistributedSpliter this name is not the same as the class name.

…_send_op

panyx0718 · 2018-05-25T01:50:53Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

+std::vector<std::string> MultiDevSSAGraphBuilder::FindDistTrainSendVars(
+    const ProgramDesc &program) const {
+  std::vector<std::string> send_vars;
+  for (auto *op : program.Block(0).AllOps()) {


Add a comment to say "since parameters are all in block 0, it's enough to only scan send ops in block 0?

panyx0718 · 2018-05-25T01:56:43Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

+    const ProgramDesc &program) const {
+  std::vector<std::string> send_vars;
+  for (auto *op : program.Block(0).AllOps()) {
+    if (op->Type() == "send_vars" || op->Type() == "send") {


We need to gradually remove these "send_vars" "send" strings. It's hard to maintain.

I added a comment here, can we fix this in anohter PR?

panyx0718 · 2018-05-25T01:59:27Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

+    const ProgramDesc &program) const {
+  std::vector<std::string> recv_vars;
+  for (auto *op : program.Block(0).AllOps()) {
+    if (op->Type() == "recv" || op->Type() == "send") {


Why "send" is here as well?

Done, delete this deprecated send op.

panyx0718 · 2018-05-25T01:59:53Z

paddle/fluid/framework/details/CMakeLists.txt

@@ -4,6 +4,7 @@ cc_library(scale_loss_grad_op_handle SRCS scale_loss_grad_op_handle.cc DEPS op_h
 cc_library(fetch_op_handle SRCS fetch_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory)


This PR needs a convergence experiment to make sure it still converge to the same place.

Sure, will do that.

panyx0718 · 2018-05-25T02:38:16Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

    return false;
  }

  /**
   * Check any of opvars contains `.block` and in sendvars
   */
  auto checker = [](const std::vector<std::string> &opvars,
-                    const std::vector<std::string> &sendvars) -> bool {
+                    const std::vector<std::string> &rpc_vars) -> bool {
    for (auto &var : opvars) {
      if (var.find(".block") != std::string::npos &&


what's the meaning of '.block'?

Dist transpiler would split the gradient var into multiple blocks, and append a suffix .block at the end of the var name, I will add some comments here to explain the hard code.

panyx0718 · 2018-05-25T02:41:26Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

        return true;
      }
    }
    return false;
  };

-  if (op.Type() == "split" || op.Type() == "split_byref") {
-    return checker(op.OutputArgumentNames(), send_op->InputArgumentNames());
+  if (op.Type() == "split" || op.Type() == "split_byref" ||


Is there a way to make these codes more maintainable?

I reviewed this PR again, and we don't need to pay attention to the op type, because in the DistributedTranspiler, we split the var before send and concat the vars after recv, so pay attention to the Inputs and Outputs is engouht.

panyx0718 · 2018-05-25T03:11:20Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

+                                                const OpDesc &op) const {
+  CreateComputationalOp(result, op, 0);
+  if (op.Type() == "concat") {
+    ConnectOp(result, result->ops_.back().get(), "fetch_barrier");


Can there be 2 fetch_barrier ops? and it connects to the wrong one?

For the current implementation of dist transpiler, there would be only one fetch_barrier_op, will also add some comments here and maybe add an argument bool connect_all in function ConnectOp would be better.

panyx0718 · 2018-05-25T03:28:36Z

paddle/fluid/operators/detail/grpc_client.cc

@@ -257,8 +257,7 @@ std::shared_ptr<grpc::Channel> RPCClient::GetChannel(const std::string& ep) {

  auto ch =
      grpc::CreateCustomChannel(ep, grpc::InsecureChannelCredentials(), args);
-
-  channels_[ep] = ch;
+  channels_[key] = ch;


Just curious, what's the benefit of having multiple channels per ep?

It's a testing feature about multi-channel and throughput, I will revert these codes and do much more test in the next PR.

…_send_op

Yancey1989 · 2018-05-28T04:20:16Z

develop branch acc logs:

Pass = 0, Training performance = 13.056212 imgs/s, Train accuracy = 0.034640, Test accuracy = 0.025490
Pass = 1, Training performance = 13.458697 imgs/s, Train accuracy = 0.042609, Test accuracy = 0.029412
Pass = 2, Training performance = 13.474373 imgs/s, Train accuracy = 0.049005, Test accuracy = 0.041176
Pass = 3, Training performance = 13.460915 imgs/s, Train accuracy = 0.053057, Test accuracy = 0.048039
Pass = 4, Training performance = 13.443091 imgs/s, Train accuracy = 0.057180, Test accuracy = 0.058824
Pass = 5, Training performance = 13.433230 imgs/s, Train accuracy = 0.059657, Test accuracy = 0.055882
Pass = 6, Training performance = 13.580382 imgs/s, Train accuracy = 0.061427, Test accuracy = 0.045098
Pass = 7, Training performance = 13.464448 imgs/s, Train accuracy = 0.063039, Test accuracy = 0.054902

overlap branch acc logs:

Pass = 0, Training performance = 14.861740 imgs/s, Train accuracy = 0.030411, Test accuracy = 0.027451
Pass = 1, Training performance = 15.150844 imgs/s, Train accuracy = 0.034640, Test accuracy = 0.046078
Pass = 2, Training performance = 15.204456 imgs/s, Train accuracy = 0.042338, Test accuracy = 0.037255
Pass = 3, Training performance = 15.216520 imgs/s, Train accuracy = 0.048626, Test accuracy = 0.047059
Pass = 4, Training performance = 15.235357 imgs/s, Train accuracy = 0.052724, Test accuracy = 0.049020
Pass = 5, Training performance = 15.267941 imgs/s, Train accuracy = 0.055998, Test accuracy = 0.062745
Pass = 6, Training performance = 15.212211 imgs/s, Train accuracy = 0.059382, Test accuracy = 0.070588
Pass = 7, Training performance = 15.209697 imgs/s, Train accuracy = 0.061555, Test accuracy = 0.067647
Pass = 8, Training performance = 15.232591 imgs/s, Train accuracy = 0.064455, Test accuracy = 0.060784

Yancey1989 · 2018-05-28T04:34:00Z

A problem was found in the process of testing distributed training, that trainer will be hung with ParallelExecutor but it works well with default Executor. This problem appears in develop branch and overlap branch, and I created an issue #10963 to follow this bug.

panyx0718

Some small comments can be fixed now. Others can be left as TODO.

panyx0718 · 2018-05-28T04:27:50Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

-    return checker(op.InputArgumentNames(), send_op->OutputArgumentNames());
+  return checker(op.OutputArgumentNames(), send_vars) ||
+         checker(op.InputArgumentNames(), recv_vars);
+  return false;


remove this line?

panyx0718 · 2018-05-28T04:39:37Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

+        "rpc op should be in [send,"
+        "send_vars, send_barrier. recv, fetch_barrier]");
+  }
+
  // FIXME(wuyi): send op always copy from GPU 0


remove this FIXME as well?

Done, and add a TODO to improve the performance.

panyx0718 · 2018-05-28T04:43:04Z

paddle/fluid/framework/details/rpc_op_handle.cc

+  // Wait input done
+  for (auto *in : inputs_) {
+    auto &p = static_cast<VarHandle *>(in)->place_;
+    if (in->DebugString() == "dummy") {  // HACK


need a better solution here soon.

Done, I add a TOOD comment here.

panyx0718 · 2018-05-28T04:51:50Z

paddle/fluid/framework/variable.h

@@ -38,6 +39,7 @@ class Variable {

  template <typename T>
  T* GetMutable() {
+    std::unique_lock<std::mutex> lock(mutex_);


Add a TODO here to make Variable completely thread-safe?

panyx0718 · 2018-05-28T04:52:17Z

paddle/fluid/inference/analysis/device.h

@@ -13,6 +13,8 @@ See the License for the specific language governing permissions and
 limitations under the License. */
 #pragma once

+#pragma once


panyx0718 · 2018-05-28T05:15:21Z

paddle/fluid/operators/send_recv_util.h

@@ -20,6 +20,9 @@ namespace operators {

 inline bool NeedSend(const framework::Scope& scope,


Should this method test if the variable is a parameter on ps server?

May not, an op don't need to care about where the parameter is(transpiler knew much more about this), only consider the variable is initialized or not.

panyx0718 · 2018-05-28T05:15:57Z

python/paddle/fluid/tests/unittests/test_dist_transpiler.py

-        return [op.type for op in trainer.global_block().ops
-                ] + ["split_byref", "send", "concat"]
+        ops = [op.type for op in trainer.global_block().ops] + [
+            "split_byref", "send_vars", "send_barrier", "recv", "recv",


recv duplicated?

No, one parameter -> one recv op, so there would be multiple send op and recv op, such as the ssa a graph of fit_a_line:

panyx0718 · 2018-05-28T05:19:49Z

python/paddle/fluid/transpiler/ps_dispatcher.py

+
+class HashName(PSDispatcher):
+    """
+      Hash variable names to servral endpoints


panyx0718 · 2018-05-28T05:25:57Z

python/paddle/fluid/transpiler/distribute_transpiler.py

+            eplist = ps_dispatcher.dispatch(splited_vars)
+            if len(splited_vars) == 1:
+                orig_varname = splited_vars[0].name
+                index = find_op_by_output_arg(program.global_block(),


Does dist_transpiler works well with memory_optimizer_transpiler? memory_optimize_transpiler tend to change var names.
leave a TODO?

Thanks for reminding, we have not test whether distributed_transpiler works well with memory_optimize_transpiler, it's a good point.

panyx0718 · 2018-05-28T05:59:34Z

python/paddle/fluid/transpiler/distribute_transpiler.py

+        #    type="fetch_barrier",
+        #    inputs={},
+        #    outputs={"RPCClient": rpc_client_var},
+        #    attrs={"endpoints": pserver_endpoints})


typhoonzero · 2018-05-28T06:13:04Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

+  for (auto *op : program.Block(0).AllOps()) {
+    // TODO(Yancey1989): use a graceful method to find send op,
+    // instead of the the hard code string
+    if (op->Type() == "send_vars") {


Can we merge send_vars and send op by adding some attributes to control the behaviour?

Please follow the comment: #10550 (comment)

typhoonzero · 2018-05-28T06:21:38Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

-    std::ostringstream sout;
-    PrintGraphviz(*graph, sout);
-    VLOG(10) << sout.str();
+    std::ofstream fout("/tmp/graph.dot");


Use GFLAG to define this output path.

typhoonzero · 2018-05-28T06:32:21Z

paddle/fluid/framework/details/rpc_op_handle.cc

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "paddle/fluid/framework/details/rpc_op_handle.h"


If send_op_handle is not used anymore, can you just rename it?

Deleted the unused send_op_handle.

typhoonzero · 2018-05-28T06:38:46Z

paddle/fluid/framework/variable.h

@@ -38,6 +39,7 @@ class Variable {

  template <typename T>
  T* GetMutable() {
+    std::unique_lock<std::mutex> lock(mutex_);


I think we can't give var a mutex here. Vars are always not thread-safe in any programming language. You must protect your vars where you use it concurrently, but not add it here.

And, adding a mutex cannot protect the var's content actually.

This can not make var thread-safe completely, only make GetMutable function thread-safe, I filed an issue to follow make variable and gRPC client thread-safe.

Related issue #10969

I think we should use a workaround instead of changing Variable implement anyway.

Maybe we can use GRPCClient instance as a singleton, or do you have some other ideas?

typhoonzero · 2018-05-28T06:39:01Z

paddle/fluid/inference/analysis/device.h

@@ -13,6 +13,8 @@ See the License for the specific language governing permissions and
 limitations under the License. */
 #pragma once

+#pragma once


Why #pragma once twice?

redundant codes, deleted it...

typhoonzero · 2018-05-28T06:40:26Z

paddle/fluid/operators/fetch_barrier_op.cc

+#include <future>  // NOLINT
+#include <ostream>
+
+#include "paddle/fluid/framework/data_type.h"


I think we might only need send and recv ops and adding attrs to control's it's behavior so that we won't copy these codes around.

Good idea, but it's hard for the current implement of SSAGraph, it's use op type and input/output argument to determine the dependency, we need to do more things to merge send_op, send_vars_op, batch_barrier, recv_op, fetch_barrier. I created an issue to follow this comment.
Related issue: #10968

typhoonzero

Please do not merge this PR until clean it up. Thanks!

…_send_op

Yancey1989 · 2018-05-29T03:39:49Z

Hi @typhoonzero , fixed the comments above, and make RPCClient as a singleton mode.

typhoonzero

LGTM++

typhoonzero · 2018-05-29T07:31:01Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

@@ -134,12 +175,14 @@ std::unique_ptr<SSAGraph> MultiDevSSAGraphBuilder::Build(

  bool is_forwarding = true;
  for (auto *op : program.Block(0).AllOps()) {
-    if (op->Type() == "send") {
-      // append send op if program is distributed trainer main program.
+    if (boost::get<int>(


typhoonzero · 2018-05-29T07:33:11Z

paddle/fluid/framework/details/multi_devices_graph_builder.cc

  auto &p = places_[0];
  auto *s = local_scopes_[0];
-  // FIXME(wuyi): send op always copy from GPU 0


Should add back this FIXME?

Will add back this comment in the next PR..

overlap sendop and backward ops

b1e5183

Yancey1989 requested review from jacquesqiao and typhoonzero May 10, 2018 03:23

Yancey1989 added 4 commits May 10, 2018 19:27

update

6e5635f

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

b35ea1a

…_send_op

add fetch_barrier_op

315e44a

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

00efc4c

…_send_op

Yancey1989 changed the title ~~[WIP]overlap send op and backward ops~~ [WIP] May 15, 2018

Yancey1989 changed the title ~~[WIP]~~ overlap send ops and backward ops May 15, 2018

Yancey1989 added 6 commits May 15, 2018 16:40

revert executor run

eb2e68e

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

274df85

…_send_op

support multiple devices

62af10d

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

952fa04

…_send_op

fix pe

e9abc66

update

147d54b

Yancey1989 requested a review from panyx0718 May 22, 2018 10:02

Yancey1989 added 2 commits May 23, 2018 11:44

connect fetch barrier and concat op

6debbcd

use req_count as atomic type

540b453

panyx0718 requested a review from chengduoZH May 23, 2018 07:22

typhoonzero reviewed May 24, 2018

View reviewed changes

Yancey1989 added 2 commits May 24, 2018 12:26

fix async worker

fc06222

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

0aa6f9e

…_send_op

Yancey1989 force-pushed the overlap_send_op branch from b47a8aa to 0aa6f9e Compare May 25, 2018 05:55

panyx0718 reviewed May 25, 2018

View reviewed changes

Yancey1989 added 2 commits May 27, 2018 18:28

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

ceefbf3

…_send_op

polish code

268e9dc

Yancey1989 force-pushed the overlap_send_op branch from bbccdd0 to 268e9dc Compare May 27, 2018 16:28

panyx0718 previously approved these changes May 28, 2018

View reviewed changes

typhoonzero reviewed May 28, 2018

View reviewed changes

typhoonzero requested changes May 28, 2018

View reviewed changes

This was referenced May 28, 2018

Merge too much kinds of RPC ops #10968

Closed

Make gRPC client thread-safe #10969

Closed

Yancey1989 added 2 commits May 28, 2018 15:37

clean up codes

ad6c014

add gflag ssa_graph_path

28596a3

Yancey1989 dismissed panyx0718’s stale review via 28596a3 May 28, 2018 07:51

Yancey1989 added 2 commits May 29, 2018 11:37

singleton rpc_client

20c24c0

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into overlap…

60d827a

…_send_op

Yancey1989 added 3 commits May 29, 2018 11:43

revert variable mutex

6b91d40

fix unit test

8b630ae

fix code style

5d7c58e

typhoonzero approved these changes May 29, 2018

View reviewed changes

typhoonzero reviewed May 29, 2018

View reviewed changes

Yancey1989 merged commit d92a75b into PaddlePaddle:develop May 29, 2018

Yancey1989 deleted the overlap_send_op branch May 29, 2018 08:01

		@@ -4,6 +4,7 @@ cc_library(scale_loss_grad_op_handle SRCS scale_loss_grad_op_handle.cc DEPS op_h
		cc_library(fetch_op_handle SRCS fetch_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory)

		@@ -20,6 +20,9 @@ namespace operators {

		inline bool NeedSend(const framework::Scope& scope,

overlap send ops and backward ops #10550

overlap send ops and backward ops #10550

Conversation

Yancey1989 commented May 10, 2018 • edited Loading

typhoonzero commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yancey1989 May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yancey1989 May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yancey1989 May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Yancey1989 commented May 28, 2018 • edited Loading

Yancey1989 commented May 28, 2018 • edited Loading

panyx0718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yancey1989 May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yancey1989 May 28, 2018 • edited Loading

Choose a reason for hiding this comment

typhoonzero left a comment

Choose a reason for hiding this comment

Yancey1989 commented May 29, 2018 • edited Loading

typhoonzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yancey1989 commented May 10, 2018 •

edited

Loading

Yancey1989 May 27, 2018 •

edited

Loading

Yancey1989 May 27, 2018 •

edited

Loading

Yancey1989 May 27, 2018 •

edited

Loading

Yancey1989 commented May 28, 2018 •

edited

Loading

Yancey1989 commented May 28, 2018 •

edited

Loading

Yancey1989 May 28, 2018 •

edited

Loading

Yancey1989 May 28, 2018 •

edited

Loading

Yancey1989 commented May 29, 2018 •

edited

Loading