Add brpc surpport. by gongweibao · Pull Request #11263 · PaddlePaddle/Paddle

gongweibao · 2018-06-07T04:29:44Z

Add only brpc framework and use a macro to avoid affecting original grpc functions.

… brpc2

typhoonzero · 2018-06-08T11:42:41Z

paddle/fluid/operators/detail/CMakeLists.txt

 endif()
+
+
+set(DISTRIBUTE_COMPILE_FLAGS "-Wno-non-virtual-dtor -Wno-error=non-virtual-dtor -Wno-error=delete-non-virtual-dtor")


Do brpc still need to set this flags?

typhoonzero · 2018-06-08T11:51:38Z

paddle/fluid/operators/test_send_nccl_id.cc

-TEST(SendNcclId, GrpcServer) {
+TEST(SendNcclId, RPCServer) {
  g_req_handler.reset(new detail::RequestSendHandler(true));
+#ifdef PADDLE_WITH_GRPC


Can use a macro somewhere in the header to define the RPCServer type to save some lines, like:

#ifdef PADDLE_WITH_GRPC #define RPCSERVER_T detail::AsyncGRPCServer #else #define RPCSERVER_T detail::AsyncBRPCServer #endif

Then use RPCSERVER_T

Tuvie · 2018-06-08T14:15:45Z

paddle/fluid/operators/detail/brpc_client.cc

+
+DEFINE_int32(brpc_channel_num, 24,
+             "Number of channels to send requests connected to one server");
+DEFINE_string(connection_type, "pooled",


I think we can directly use "pooled" connection type. "single" means that we can use at most one connection to one server.

Thanks! Done!

Tuvie · 2018-06-08T14:22:08Z

paddle/fluid/operators/detail/brpc_client.cc

+             "Number of channels to send requests connected to one server");
+DEFINE_string(connection_type, "pooled",
+              "Connection type. Available values: single, pooled, short");
+DEFINE_int32(timeout_ms, -1, "RPC timeout in milliseconds");


Since we have time_out specified in AsyncSendVar, it seems that we do not need this flag.

Is there a way to apply timeout to one send or recv action instead of to channel options?

You can set_timeout_ms() to configure controller, which works for the current RPC call.

Tuvie · 2018-06-08T14:34:33Z

paddle/fluid/operators/detail/brpc_client.cc

+        sendrecv::VoidMessage* response = new sendrecv::VoidMessage();
+
+        google::protobuf::Closure* done =
+            brpc::NewCallback(&HandleSendResponse, cntl, response);


This callback is called when the RPC is finished, which means that the client have got the response. It looks like a misuse.

So it's ok to handle the response?

Maybe I misunderstand the meaning of HandleSendResponse. Does it refer to the handler after getting the response of "Send" RPC call? If it is, there is no problem here.

Ye, it's a handler. The name should be polished. Thanks!

panyx0718 · 2018-06-09T08:04:47Z

cmake/external/leveldb.cmake

@@ -0,0 +1,44 @@
+# Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved.


Is there anyway to avoid depending on this?

I will delete this if I found a way to avoid depending on this.

What is the reason for depending on it now?

https://github.com/brpc/brpc/blob/master/CMakeLists.txt#L116

@Tuvie Is there anyway for brpc to avoid depending on leveldb?

panyx0718 · 2018-06-09T08:06:45Z

paddle/fluid/operators/detail/brpc_client.cc

+                        sendrecv::VoidMessage* response) {
+  // std::unique_ptr makes sure cntl/response will be deleted before returning.
+  std::unique_ptr<brpc::Controller> cntl_guard(cntl);
+  std::unique_ptr<sendrecv::VoidMessage> response_guard(response);


usually we can just make std::unique_ptr<brpc::Controller> cntl argument

It should be copied.

typhoonzero

almost LGTM!

typhoonzero · 2018-06-11T01:42:12Z

paddle/fluid/operators/detail/brpc_server.cc

+namespace sendrecv {
+class BRPCServiceImpl : public SendRecvService {
+ public:
+  BRPCServiceImpl(const std::unordered_map<


Can use typedef std::unordered_map<std::string, paddle::operators::detail::RequestHandler*> HandlerMap to simplify the code.

Done. Thanks!

typhoonzero · 2018-06-11T01:46:21Z

paddle/fluid/operators/detail/brpc_server.cc

+                    google::protobuf::Closure* done) override {
+    brpc::ClosureGuard done_guard(done);
+
+    paddle::framework::Scope* local_scope = request_send_h_->scope();


request_send_h_ may be nullptr? well in some rare cases, but request_prefetch_h_ may be null in most cases. Better to add a check here?

Set nullptr when initialize.

Well, if request_prefetch_h_ is not set actually, request_prefetch_h_->scope() will cause error.

I add checker when they're called.

typhoonzero · 2018-06-11T01:52:14Z

paddle/fluid/operators/detail/send_recv.proto

 syntax = "proto3";
 package sendrecv;

+// option cc_generic_services = true;


Can delete this if not needed? Curious what will this line affect?

https://developers.google.com/protocol-buffers/docs/proto

Whether or not the protocol buffer compiler should generate abstract service code based on services definitions in C++, Java, and Python, respectively. For legacy reasons, these default to true. However, as of version 2.3.0 (January 2010), it is considered preferrable for RPC implementations to provide code generator plugins to generate code more specific to each system, rather than rely on the "abstract" services

Grpc uses the code generator plugin to generate more specific to each system, and it needs this to be false. While brpc uses the default generated abstract service code, and it needs this to be true. I will fix this to be compatbile in next PR.

Thanks for this information!

typhoonzero · 2018-06-11T01:54:04Z

paddle/fluid/operators/listen_and_serv_op.cc

  rpc_service_->SavePort();
 }

+static int64_t GetTimestamp() {


This is already defined in sendrecvop_utils.h can delete codes there.

If sendrecvop_utils.h is included in files it needs to know grpc types explicitly.
So I move this function's definition here and it's used only in this file now.

panyx0718 · 2018-06-11T05:55:19Z

paddle/fluid/operators/gen_nccl_id_op.cc

-    detail::AsyncGRPCServer rpc_service(endpoint, 1);
-    rpc_service.RegisterRPC(detail::kRequestSend, &rpc_h);
-    rpc_h.SetRPCServer(&rpc_service);
+    std::unique_ptr<detail::RPCServer> rpc_service(nullptr);


no need for nullptr

Thanks. Done.

panyx0718 · 2018-06-11T05:58:23Z

paddle/fluid/operators/detail/brpc_server.h

+#include "paddle/fluid/operators/detail/request_handler.h"
+#include "paddle/fluid/operators/detail/rpc_server.h"
+#include "paddle/fluid/operators/detail/send_recv.pb.h"
+#include "paddle/fluid/platform/profiler.h"


It seems you don't need to include most of these things in header?

Thanks.Done.

panyx0718 · 2018-06-11T06:11:15Z

paddle/fluid/operators/detail/brpc_client.h

+    }
+  }
+  brpc::Channel channel;
+  sendrecv::SendRecvService_Stub* stub;


why not make it unique_ptr?

It's need to be copied.

Can use shared_ptr here?

panyx0718 · 2018-06-11T06:14:20Z

paddle/fluid/operators/detail/brpc_client.cc

+
+DEFINE_int32(brpc_channel_num, 24,
+             "Number of channels to send requests connected to one server");
+DEFINE_int32(timeout_ms, -1, "RPC timeout in milliseconds");


why is default -1?

Done.Thanks!

panyx0718 · 2018-06-11T06:25:15Z

cmake/external/leveldb.cmake

@@ -0,0 +1,44 @@
+# Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved.


@Tuvie Is there anyway for brpc to avoid depending on leveldb?

typhoonzero

LGTM!

gongweibao added 14 commits June 5, 2018 08:19

init

83ccc80

add brpc

acce88c

add brpc

e586cc7

add brpc files

67e75b0

fix brpc completion error

59db54d

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

380749f

… brpc2

add brpc

c610343

add

392af8f

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

73604c0

… brpc2

add brpc proto

49a9501

add brpc

604f8ba

clean up

3e0df1e

fix compilation error

335fc93

fix

29bfa02

gongweibao requested review from jacquesqiao, panyx0718 and typhoonzero June 8, 2018 06:23

gongweibao changed the title ~~[WIP]Add brpc surpport.~~ Jun 8, 2018

gongweibao requested a review from Yancey0623 June 8, 2018 06:34

typhoonzero reviewed Jun 8, 2018

View reviewed changes

Tuvie reviewed Jun 8, 2018

View reviewed changes

panyx0718 reviewed Jun 9, 2018

View reviewed changes

gongweibao added 2 commits June 10, 2018 02:57

follow comments

6b3cb0d

follow comments

a21b41e

typhoonzero reviewed Jun 11, 2018

View reviewed changes

panyx0718 reviewed Jun 11, 2018

View reviewed changes

gongweibao added 2 commits June 11, 2018 07:39

follow comments

e7bf164

shared ptr

381776b

merge

438fc07

typhoonzero approved these changes Jun 11, 2018

View reviewed changes

gongweibao merged commit d9de6b8 into PaddlePaddle:develop Jun 11, 2018

gongweibao deleted the brpc2 branch June 11, 2018 12:25

		endif()


		set(DISTRIBUTE_COMPILE_FLAGS "-Wno-non-virtual-dtor -Wno-error=non-virtual-dtor -Wno-error=delete-non-virtual-dtor")

		@@ -0,0 +1,44 @@
		# Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved.

Conversation

gongweibao commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

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

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

gongweibao Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

typhoonzero left a comment

Choose a reason for hiding this comment

Labels

4 participants

gongweibao commented Jun 7, 2018 •

edited

Loading

gongweibao Jun 11, 2018 •

edited

Loading

gongweibao Jun 11, 2018 •

edited

Loading