Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/extract op info into op info.cc #3587

Merged

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Aug 20, 2017

Fix cycle dependencies, Fix #3583.

Fix cycle dependencies, Fix PaddlePaddle#3583.
* Add Get/Has methods to OpInfoMap
* Add PADDLE_ENFORCE for OpInfo to get field.
Copy link
Collaborator

@wangkuiyi wangkuiyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我没看明白为什么需要 OpInfo 和 OpInfoMap 。它们和 OpRegistry 是什么关系?尤其是OpReistry里有不少singleton了,为什么还要再加一个OpInfoMap::Instance?

return proto_ != nullptr && checker_ != nullptr;
}

const OpProto& Proto() const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if these PADDLE_ENFORCE are necessary. But if they are, please make OpInfo a class instead of a struct due to https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes. Especially, if we need OpInfo a class, we should declare creator_ and other data members private.

bool HasGradientOp() const { return !grad_op_type_.empty(); }
};

class OpInfoMap {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class seems unnecessary and should be removed, as its methods basically duplicate unordered_map's interface.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lot duplicated code before.

See code here. To use unordered_map directly could be nosing to check Is that key in the map? If not, enforce an error again and again.


static OpInfoMap* g_op_info_map = nullptr;

OpInfoMap& OpInfoMap::Instance() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right way to implement a singleton in C++ doesn't need global pointer variables:

OpInfoMap& OpInfoMap::Singleton() {
  static OpInfoMap _;
  return _;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

In Google C++ style, Only POD type can be a global variable. OpInfoMap is a class, it should not be a global variable.

Copy link
Collaborator

@wangkuiyi wangkuiyi Aug 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

确实。不过看上去也不需要一个global variable,可以是这样?

OpInfoMap& OpInfoMap::Singleton() {
  static OpInfoMap* p = new OpInfoMap;
  return *p;
}

@reyoung
Copy link
Collaborator Author

reyoung commented Aug 21, 2017

我没看明白为什么需要 OpInfo 和 OpInfoMap 。它们和 OpRegistry 是什么关系?尤其是OpReistry里有不少singleton了,为什么还要再加一个OpInfoMap::Instance?

@wangkuiyi OpInfo和OpInfoMap并不是新增的概念。他们原来在OpRegistry中。OpRegistry里面的singleton都统一到这个OpInfoMap::Instance里了。

这么做的原因如#3583所示,主要是:

  1. OpInfo,特别是其中的OpProto,应该是Operator自身的信息。Operator里面有一些操作需要知道自己的OpProto是什么。譬如Op中如果要知道哪一个输出是duplicable的,需要查询这个Op的OpProto中的outputs数组。在原来的实现中,有一个循环引用问题

    • Operator需要查询OpInfo来完成部分操作。
    • OpRegistry保存了OpInfo,但是需要依赖Operator模块进行注册。
    • 即 Operator依赖OpInfo(存在OpRegistry中),OpRegistry依赖Operator。
  2. 原来的singleton不符合google C++ style。

  3. code中有很多重复的代码来去判断:

    • 某个Op是不是注册过。
    • 某个Op的Gradient是不是注册过。
    • 某个Op有没有OpProto。
    • 等等。

#define DEFINE_OP_CONSTRUCTOR(CLS, PARENT_CLS) \
CLS(const std::string& type, const VarNameMap& inputs, \
const VarNameMap& outputs, const paddle::framework::AttributeMap& attrs) \
#define DEFINE_OP_CONSTRUCTOR(CLS, PARENT_CLS) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CLS and PARENT_CLS should be lowercase.
See this example from Google c++ code style: https://google.github.io/styleguide/cppguide.html#Macro_Names

#define ROUND(x) ...

x is lowercase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@reyoung reyoung merged commit 484a2da into PaddlePaddle:develop Aug 23, 2017
@reyoung reyoung deleted the feature/extract_op_info_into_op_info.cc branch October 2, 2017 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants