Feature/remove shared ptr #3538
Conversation
* Also simplify pybind implementation by using OperatorBase as holder type.
| @@ -91,7 +89,7 @@ std::shared_ptr<OperatorBase> BackwardRecursive( | |||
| } | |||
|
|
|||
| // Returned gradient network | |||
| auto net = std::make_shared<operators::NetOp>(); | |||
| auto net = std::unique_ptr<operators::NetOp>(new operators::NetOp()); | |||
There was a problem hiding this comment.
can we have a make_unique helper ?
There was a problem hiding this comment.
Well, it could be in another PR. make_unique is not in C++ 11 but in C++ 14. We can implement it in C++ 11.
paddle/framework/operator.h
Outdated
| protected: | ||
| std::string type_; | ||
| // NOTE: in case of OpGrad, inputs_ contains: | ||
| // I (Inputs) | ||
| // I (Inputs)opear |
paddle/operators/net_op.h
Outdated
| } | ||
|
|
||
| void InsertOp(size_t pos, const std::shared_ptr<OperatorBase>& op) { | ||
| void InsertOp(size_t pos, framework::OperatorBase* op, bool own) { |
There was a problem hiding this comment.
what's the case when a net does't own an inserted op?
There was a problem hiding this comment.
It means that op is owned by another place (other network or owned by Python).
In that situation, the op will be Cloned first.
There was a problem hiding this comment.
In caffe2's design, every net own its ops? If same op is added, just clone/(create new and copy) it.
If an op belongs to python, python will get a unique_ptr to help control the memory. When net want to add a same op, just clone (create new and copy) it simpler?
There was a problem hiding this comment.
Every NetOp own operators in this design. The own parameter actually means is this pointer could be moved into NetOp and NetOp will take care that pointer's life cycle.
Maybe that API is confusing. It is removed now.
paddle/framework/operator.h
Outdated
| // Macro for define a clone method. | ||
| // If you are writing an kernel operator, `Clone` will be defined when you | ||
| // register it. i.e. `Clone` method is not needed to define by yourself. | ||
| #define DEFINE_OP_CLONE_METHOD(CLS) \ |
There was a problem hiding this comment.
registry里边的macro的参数用了 xx_yy这种命名风格，这里是不是应该保持一致？比如叫做 op_class
There was a problem hiding this comment.
那是因为registry里面有很多class同时在一个macro，比如op_class, op_type, grad_class, etc.
paddle/framework/operator.h
Outdated
| // You can also use | ||
| // using PARENT_CLASS::PARENT_CLASS; | ||
| // to use parent's constructor. | ||
| #define DEFINE_OP_CONSTRUCTOR(CLS, PARENT_CLS) \ |
There was a problem hiding this comment.
这里的命名同上，我看了下caffe2和tensorflow的代码，都是使用了 xx_yy这种方法命名宏的参数，所以要不咱们也按照这个规范来？
There was a problem hiding this comment.
这并没有在Google C++ code style中有描述。
paddle/framework/operator.h
Outdated
| class NOP : public OperatorBase { | ||
| public: | ||
| using OperatorBase::OperatorBase; | ||
| void InferShape(const Scope& scope) const override {} | ||
| void Run(const Scope& scope, | ||
| const platform::DeviceContext& dev_ctx) const override {} | ||
| std::unique_ptr<OperatorBase> Clone() const override { |
There was a problem hiding this comment.
这里为啥没有统一使用DEFINE_OP_CLONE_METHOD?
There was a problem hiding this comment.
DEFINE_OP_CLONE_METHOD是一个只会被最子类使用的macro，这个函数被标记为final了。
这里，有一些类型会继承NOP，所以不能将Clone标记为final
* add_op could take a unique_ptr or a const reference. If unique_ptr is taken, the NetOp will take care of that operator's life cycle. If a const reference is taken, that op will be Cloned.
Fit Google C++ Style
… into feature/remove_shared_ptr
This PR is the second step of issue #3520.
This PR is following PR #3522 . Please Review #3522 First.