From 98c12b1a087bd719d247df6908294bd63cbf5abe Mon Sep 17 00:00:00 2001 From: Xin Pan Date: Thu, 19 Apr 2018 01:09:40 +0800 Subject: [PATCH] Clean up C++ codes. (#10022) * Privatize OpHandleBase * Clean up a few private members --- .../framework/details/broadcast_op_handle.h | 8 ++-- .../details/broadcast_op_handle_test.cc | 2 +- .../details/computation_op_handle.cc | 4 +- .../framework/details/computation_op_handle.h | 13 ++++-- .../fluid/framework/details/fetch_op_handle.h | 15 ++++--- .../framework/details/gather_op_handle.h | 8 ++-- .../details/gather_op_handle_test.cc | 2 +- .../details/multi_devices_graph_builder.cc | 3 +- .../details/nccl_all_reduce_op_handle.h | 9 ++-- .../fluid/framework/details/op_handle_base.h | 41 ++++++++++++------- .../details/scale_loss_grad_op_handle.h | 11 +++-- .../fluid/framework/details/send_op_handle.h | 9 ++-- .../framework/details/ssa_graph_builder.cc | 6 +-- .../details/threaded_ssa_graph_executor.cc | 8 ++-- 14 files changed, 85 insertions(+), 54 deletions(-) diff --git a/paddle/fluid/framework/details/broadcast_op_handle.h b/paddle/fluid/framework/details/broadcast_op_handle.h index b329242252..bc3e373488 100644 --- a/paddle/fluid/framework/details/broadcast_op_handle.h +++ b/paddle/fluid/framework/details/broadcast_op_handle.h @@ -29,9 +29,7 @@ namespace framework { namespace details { struct BroadcastOpHandle : public OpHandleBase { - const std::vector &local_scopes_; - const std::vector &places_; - + public: BroadcastOpHandle(const std::vector &local_scopes, const std::vector &places); @@ -41,6 +39,10 @@ struct BroadcastOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + const std::vector &local_scopes_; + const std::vector &places_; }; } // namespace details diff --git a/paddle/fluid/framework/details/broadcast_op_handle_test.cc b/paddle/fluid/framework/details/broadcast_op_handle_test.cc index bcd61335be..efc7051582 100644 --- a/paddle/fluid/framework/details/broadcast_op_handle_test.cc +++ b/paddle/fluid/framework/details/broadcast_op_handle_test.cc @@ -90,7 +90,7 @@ struct TestBroadcastOpHandle { op_handle_->AddInput(dummy_var_handle); for (size_t j = 0; j < gpu_list_.size(); ++j) { - op_handle_->dev_ctxes_[gpu_list_[j]] = ctxs_[j].get(); + op_handle_->SetDeviceContext(gpu_list_[j], ctxs_[j].get()); VarHandle* out_var_handle = new VarHandle(2, j, "out", gpu_list_[j]); vars_.emplace_back(out_var_handle); op_handle_->AddOutput(out_var_handle); diff --git a/paddle/fluid/framework/details/computation_op_handle.cc b/paddle/fluid/framework/details/computation_op_handle.cc index ff6d91c1da..7ff0efe093 100644 --- a/paddle/fluid/framework/details/computation_op_handle.cc +++ b/paddle/fluid/framework/details/computation_op_handle.cc @@ -28,8 +28,8 @@ ComputationOpHandle::ComputationOpHandle(const OpDesc &op_desc, Scope *scope, void ComputationOpHandle::RunImpl() { auto *cur_ctx = dev_ctxes_[place_]; for (auto *in : inputs_) { - bool need_wait = - in->generated_op_ && in->generated_op_->dev_ctxes_[place_] != cur_ctx; + bool need_wait = in->generated_op_ && + in->generated_op_->DeviceContext(place_) != cur_ctx; if (need_wait) { in->generated_op_->Wait(cur_ctx); } diff --git a/paddle/fluid/framework/details/computation_op_handle.h b/paddle/fluid/framework/details/computation_op_handle.h index d6d2d731ca..c363b973d9 100644 --- a/paddle/fluid/framework/details/computation_op_handle.h +++ b/paddle/fluid/framework/details/computation_op_handle.h @@ -14,6 +14,9 @@ #pragma once +#include +#include + #include "paddle/fluid/framework/details/op_handle_base.h" #include "paddle/fluid/framework/op_registry.h" #include "paddle/fluid/framework/operator.h" @@ -24,10 +27,7 @@ namespace paddle { namespace framework { namespace details { struct ComputationOpHandle : public OpHandleBase { - std::unique_ptr op_; - Scope *scope_; - platform::Place place_; - + public: ComputationOpHandle(const OpDesc &op_desc, Scope *scope, platform::Place place); @@ -35,6 +35,11 @@ struct ComputationOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + std::unique_ptr op_; + Scope *scope_; + platform::Place place_; }; } // namespace details } // namespace framework diff --git a/paddle/fluid/framework/details/fetch_op_handle.h b/paddle/fluid/framework/details/fetch_op_handle.h index 904b2d669f..b49f3df338 100644 --- a/paddle/fluid/framework/details/fetch_op_handle.h +++ b/paddle/fluid/framework/details/fetch_op_handle.h @@ -14,6 +14,9 @@ #pragma once +#include +#include + #include "paddle/fluid/framework/details/op_handle_base.h" #include "paddle/fluid/framework/feed_fetch_type.h" #include "paddle/fluid/framework/scope.h" @@ -24,11 +27,7 @@ namespace framework { namespace details { struct FetchOpHandle : public OpHandleBase { - FeedFetchList *data_; - size_t offset_; - std::vector *local_scopes_; - std::vector tensors_; - + public: FetchOpHandle(FeedFetchList *data, size_t offset, std::vector *local_scopes); @@ -42,6 +41,12 @@ struct FetchOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + FeedFetchList *data_; + size_t offset_; + std::vector *local_scopes_; + std::vector tensors_; }; } // namespace details diff --git a/paddle/fluid/framework/details/gather_op_handle.h b/paddle/fluid/framework/details/gather_op_handle.h index 6c0231f642..d11ef8556a 100644 --- a/paddle/fluid/framework/details/gather_op_handle.h +++ b/paddle/fluid/framework/details/gather_op_handle.h @@ -29,9 +29,7 @@ namespace framework { namespace details { struct GatherOpHandle : public OpHandleBase { - const std::vector &local_scopes_; - const std::vector &places_; - + public: GatherOpHandle(const std::vector &local_scopes, const std::vector &places); @@ -41,6 +39,10 @@ struct GatherOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + const std::vector &local_scopes_; + const std::vector &places_; }; } // namespace details diff --git a/paddle/fluid/framework/details/gather_op_handle_test.cc b/paddle/fluid/framework/details/gather_op_handle_test.cc index 2da8c89d2d..9481579f6c 100644 --- a/paddle/fluid/framework/details/gather_op_handle_test.cc +++ b/paddle/fluid/framework/details/gather_op_handle_test.cc @@ -78,7 +78,7 @@ struct TestGatherOpHandle { op_handle_.reset(new GatherOpHandle(local_scopes_, gpu_list_)); // add input for (size_t j = 0; j < gpu_list_.size(); ++j) { - op_handle_->dev_ctxes_[gpu_list_[j]] = ctxs_[j].get(); + op_handle_->SetDeviceContext(gpu_list_[j], ctxs_[j].get()); auto* in_var_handle = new VarHandle(1, j, "input", gpu_list_[j]); vars_.emplace_back(in_var_handle); op_handle_->AddInput(in_var_handle); diff --git a/paddle/fluid/framework/details/multi_devices_graph_builder.cc b/paddle/fluid/framework/details/multi_devices_graph_builder.cc index d2b6a35a5d..002952436e 100644 --- a/paddle/fluid/framework/details/multi_devices_graph_builder.cc +++ b/paddle/fluid/framework/details/multi_devices_graph_builder.cc @@ -60,7 +60,8 @@ void MultiDevSSAGraphBuilder::CreateOpHandleIOs(SSAGraph *result, const platform::Place &p, const size_t &i) const { auto *op_handle = result->ops_.back().get(); - op_handle->dev_ctxes_[p] = platform::DeviceContextPool::Instance().Get(p); + op_handle->SetDeviceContext(p, + platform::DeviceContextPool::Instance().Get(p)); auto var_names = op.InputArgumentNames(); diff --git a/paddle/fluid/framework/details/nccl_all_reduce_op_handle.h b/paddle/fluid/framework/details/nccl_all_reduce_op_handle.h index ad14a3c5cb..a0c321843e 100644 --- a/paddle/fluid/framework/details/nccl_all_reduce_op_handle.h +++ b/paddle/fluid/framework/details/nccl_all_reduce_op_handle.h @@ -27,10 +27,6 @@ namespace framework { namespace details { struct NCCLAllReduceOpHandle : public OpHandleBase { - const std::vector &local_scopes_; - const std::vector &places_; - const platform::NCCLContextMap &nccl_ctxs_; - NCCLAllReduceOpHandle(const std::vector &local_scopes, const std::vector &places, const platform::NCCLContextMap &ctxs); @@ -43,6 +39,11 @@ struct NCCLAllReduceOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + const std::vector &local_scopes_; + const std::vector &places_; + const platform::NCCLContextMap &nccl_ctxs_; }; } // namespace details diff --git a/paddle/fluid/framework/details/op_handle_base.h b/paddle/fluid/framework/details/op_handle_base.h index a9a6c8d39c..00f213f3ed 100644 --- a/paddle/fluid/framework/details/op_handle_base.h +++ b/paddle/fluid/framework/details/op_handle_base.h @@ -27,28 +27,15 @@ namespace details { constexpr char kLocalExecScopeName[] = "@LCOAL_SCOPE@"; class OpHandleBase { - private: - DISABLE_COPY_AND_ASSIGN(OpHandleBase); - public: - std::vector inputs_; - std::vector outputs_; - std::unordered_map - dev_ctxes_; - -#ifdef PADDLE_WITH_CUDA - std::unordered_map events_; -#endif - OpHandleBase() {} + virtual ~OpHandleBase(); + std::string DebugString() const; virtual std::string Name() const = 0; - virtual ~OpHandleBase(); - void Run(bool use_event); virtual void Wait(platform::DeviceContext *waited_dev); @@ -61,6 +48,18 @@ class OpHandleBase { // will likely block other computations. virtual bool IsMultiDeviceTransfer() { return false; } + const platform::DeviceContext *DeviceContext(platform::Place place) { + return dev_ctxes_[place]; + } + + void SetDeviceContext(platform::Place place, platform::DeviceContext *ctx_) { + dev_ctxes_[place] = ctx_; + } + + const std::vector &Inputs() const { return inputs_; } + + const std::vector &Outputs() const { return outputs_; } + protected: void RunAndRecordEvent(const std::function &callback); @@ -68,6 +67,18 @@ class OpHandleBase { const std::function &callback); virtual void RunImpl() = 0; + + std::vector inputs_; + std::vector outputs_; + std::unordered_map + dev_ctxes_; + +#ifdef PADDLE_WITH_CUDA + std::unordered_map events_; +#endif + + DISABLE_COPY_AND_ASSIGN(OpHandleBase); }; } // namespace details diff --git a/paddle/fluid/framework/details/scale_loss_grad_op_handle.h b/paddle/fluid/framework/details/scale_loss_grad_op_handle.h index ab7353a4fc..d93d599d46 100644 --- a/paddle/fluid/framework/details/scale_loss_grad_op_handle.h +++ b/paddle/fluid/framework/details/scale_loss_grad_op_handle.h @@ -14,6 +14,8 @@ #pragma once +#include + #include "paddle/fluid/framework/details/op_handle_base.h" #include "paddle/fluid/framework/lod_tensor.h" #include "paddle/fluid/framework/scope.h" @@ -23,10 +25,6 @@ namespace framework { namespace details { struct ScaleLossGradOpHandle : public OpHandleBase { - float coeff_; - Scope *scope_; - platform::Place place_; - ScaleLossGradOpHandle(size_t num_dev, Scope *scope, platform::Place place, platform::DeviceContext *context); @@ -36,6 +34,11 @@ struct ScaleLossGradOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + float coeff_; + Scope *scope_; + platform::Place place_; }; } // namespace details diff --git a/paddle/fluid/framework/details/send_op_handle.h b/paddle/fluid/framework/details/send_op_handle.h index 173f9d7261..2f78811fad 100644 --- a/paddle/fluid/framework/details/send_op_handle.h +++ b/paddle/fluid/framework/details/send_op_handle.h @@ -28,10 +28,6 @@ namespace framework { namespace details { struct SendOpHandle : public OpHandleBase { - std::unique_ptr op_; - const Scope* local_scope_; - const platform::Place& place_; - SendOpHandle(const framework::OpDesc& op_desc, const Scope* local_scope, const platform::Place& place); @@ -43,6 +39,11 @@ struct SendOpHandle : public OpHandleBase { protected: void RunImpl() override; + + private: + std::unique_ptr op_; + const Scope* local_scope_; + const platform::Place& place_; }; } // namespace details diff --git a/paddle/fluid/framework/details/ssa_graph_builder.cc b/paddle/fluid/framework/details/ssa_graph_builder.cc index 25e8c77bb4..6a56752755 100644 --- a/paddle/fluid/framework/details/ssa_graph_builder.cc +++ b/paddle/fluid/framework/details/ssa_graph_builder.cc @@ -117,12 +117,12 @@ void SSAGraphBuilder::PrintGraphviz(const SSAGraph &graph, std::ostream &sout) { std::string op_name = "op_" + std::to_string(op_id++); sout << op_name << " [label=\"" << op->Name() << "\", shape=rect]" << std::endl; - for (auto in : op->inputs_) { + for (auto in : op->Inputs()) { std::string var_name = "var_" + std::to_string(vars[in]); sout << var_name << " -> " << op_name << std::endl; } - for (auto out : op->outputs_) { + for (auto out : op->Outputs()) { std::string var_name = "var_" + std::to_string(vars[out]); sout << op_name << " -> " << var_name << std::endl; } @@ -133,7 +133,7 @@ void SSAGraphBuilder::PrintGraphviz(const SSAGraph &graph, std::ostream &sout) { void SSAGraphBuilder::AddOutputToLeafOps(SSAGraph *graph) { for (auto &op : graph->ops_) { - if (!op->outputs_.empty()) { + if (!op->Outputs().empty()) { continue; } auto *dummy_leaf = new DummyVarHandle(); diff --git a/paddle/fluid/framework/details/threaded_ssa_graph_executor.cc b/paddle/fluid/framework/details/threaded_ssa_graph_executor.cc index 3d2bd633af..14e75e7b7b 100644 --- a/paddle/fluid/framework/details/threaded_ssa_graph_executor.cc +++ b/paddle/fluid/framework/details/threaded_ssa_graph_executor.cc @@ -53,7 +53,7 @@ FeedFetchList ThreadedSSAGraphExecutor::Run( }; auto InsertPendingOp = [&pending_ops](OpHandleBase &op_instance) { - pending_ops.insert({&op_instance, op_instance.inputs_.size()}); + pending_ops.insert({&op_instance, op_instance.Inputs().size()}); }; // Transform SSAGraph to pending_ops & pending_vars @@ -69,7 +69,7 @@ FeedFetchList ThreadedSSAGraphExecutor::Run( } for (auto &op : graph_->ops_) { - if (op->inputs_.empty()) { // Special case, Op has no input. + if (op->Inputs().empty()) { // Special case, Op has no input. ready_ops.insert(op.get()); } else { InsertPendingOp(*op); @@ -99,7 +99,7 @@ FeedFetchList ThreadedSSAGraphExecutor::Run( fetch_ops.emplace_back(op); for (auto &p : places_) { - op->dev_ctxes_[p] = fetch_ctxs_.Get(p); + op->SetDeviceContext(p, fetch_ctxs_.Get(p)); } for (auto *var : vars) { @@ -180,7 +180,7 @@ void ThreadedSSAGraphExecutor::RunOp( op->Run(use_event_); VLOG(10) << op << " " << op->Name() << " Done "; running_ops_--; - ready_var_q->Extend(op->outputs_); + ready_var_q->Extend(op->Outputs()); VLOG(10) << op << " " << op->Name() << "Signal posted"; } catch (platform::EnforceNotMet ex) { exception_.reset(new platform::EnforceNotMet(ex)); -- GitLab