optimize attr default value by wanghuancoder · Pull Request #33357 · PaddlePaddle/Paddle

wanghuancoder · 2021-06-04T10:29:12Z

PR types

Performance optimization

PR changes

Others

Describe

1. 背景

每个OP有很多的Attribute，这些Attribute在OP Maker中设置默认值和Checker。在上层使用时，通常只传递少部分Attribute，而更多的，需要靠Checker补填默认值。这部分逻辑在TypedAttrChecker中。
而Attribute的存储方式是using AttributeMap = std::unordered_map<std::string, Attribute>;，经测试发现，这种数据结构的补填、拷贝都非常慢。在动态图中，直接增加了框架的调度成本。

2. 内容

本PR优化Attribute补填默认值带来的性能开销。

主要方法是： 不再向上层传入的AttrsMap中补填默认值。而是在注册前向OP时生成DefaultAttrMap，DefaultAttrMap用于存储OP的默认值。在传递AttrsMap时，伴随传递DefaultAttrMap，在查询Attr时，先从AttrsMap中查找，再从DefaultAttrMap中查找。对于GradOP和DoubleGradOP，其Attr为前向OP的Attr，因此在执行dygraph_grad_op_maker_时，不仅传入AttrsMap，也传入DefaultAttrMap，最终存储在OPBase中。

3. 性能测试

使用core.ops.elementwise_add测试，本PR对core.ops.elementwise_add有18.93%的性能提升。

paddle-bot-old · 2021-06-04T10:29:23Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

phlrain · 2021-06-08T09:40:59Z

    }

-    OpBase::Run(*op, new_ins, outs, attrs, place);
+    OpBase::Run(*op, new_ins, outs, attrs, attr_checker->GetAttrDefaultMap(),


attr_checker 需要判断是不是存在null的情况

已修改，感谢！

paddle-bot-old · 2021-06-16T02:35:20Z

Sorry to inform you that d44aeb6's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

phlrain · 2021-06-18T08:18:31Z

+    if (it == attrs_.end()) {
+      if (attrs_default_ != nullptr) {
+        it = attrs_default_->find(name);
+        if (it == attrs_default_->end()) {


bool found = false；

if ( it == attrs_ends() )
{
if (attrs_default_ != nullptr) {
it = attrs_default_->find(name);
if (it == attrs_default_->end())
{
found = false;
}
}

PADDLE_ENFORE( found == true, "")

phlrain · 2021-06-18T08:28:26Z

+            platform::errors::InvalidArgument(
+                "Attribute (%s) is not set correctly.", attr_name_));
+        // default_value_setter_ has no more than one element
+        attr_map->emplace(attr_name_, default_value_setter_[0]());


auto res = attr_map->emplace(attr_name_, default_value_setter_0);
it = res.first;

emplace 之后会返回一个pair，这个pair的第一个元素是新的iterator

phlrain · 2021-06-18T08:28:50Z

+        // default_value_setter_ has no more than one element
+        attr_map->emplace(attr_name_, default_value_setter_[0]());
+      }
+      it = attr_map->find(attr_name_);


有了上面的操作后，这个find可以省略

phlrain · 2021-06-18T08:31:00Z

      .SetDefault("");
  Validate();
+
+  op_checker_->InitDefaultMap();


这个Init可以放在68行后面，后面这些信息动态图都是不需要的

phlrain · 2021-06-18T08:32:31Z

  void ClearGradVarBase() { grad_var_ = nullptr; }

-  void SetGradVarBase(VarBase& grad_var) {
+  void SetGradVarBase(VarBase& grad_var) {  // NOLINT


why this line

使用const &或者*，避免NOLINT

… attr2

chenwhql

个人觉得，DefaultAttr相比AttrDefault更符合英语的修饰习惯

chenwhql · 2021-06-21T02:32:43Z

+  explicit AttrReader(const AttributeMap& attrs)
+      : attrs_(attrs), attrs_default_(nullptr) {}
+
+  AttrReader(const AttributeMap& attrs, const AttributeMap& attrs_default)


命名有些奇怪，建议attrs_default -> default_attrs

chenwhql · 2021-06-21T02:34:08Z

    explicit_checker_num_ = attr_checkers_.size();
  }

+  void InitDefaultMap() {


InitDefaultMap意义不明，InitDefaultMap -> InitDefaultAttributeMap

chenwhql · 2021-06-21T02:34:30Z


+  void InitDefaultMap() {
+    for (const auto& checker : attr_checkers_) {
+      checker(&attrs_default_, true, false);


attrs_default_ -> default_attrs_

chenwhql · 2021-06-21T02:35:34Z

+    }
+  }
+
+  const AttributeMap& GetAttrDefaultMap() const { return attrs_default_; }


GetAttrDefaultMap -> GetDefaultAttrMap

chenwhql · 2021-06-21T02:36:20Z

  }

+  // Only for dygraph
+  void SetDygraphAttrsDefaultMap(const framework::AttributeMap& attrs_default) {


SetDygraphAttrsDefaultMap -> SetDygraphDefaultAttrsMap

chenwhql · 2021-06-21T02:39:37Z

+      it = attrs_default_.find(name);
+      if (it == attrs_default_.end()) {
+        PADDLE_THROW(
+            platform::errors::NotFound("Can not find [%s] in attrs", name));


chenwhql · 2021-06-21T02:41:16Z

  void ClearGradVarBase() { grad_var_ = nullptr; }

-  void SetGradVarBase(VarBase& grad_var) {
+  void SetGradVarBase(VarBase& grad_var) {  // NOLINT


使用const &或者*，避免NOLINT

chenwhql · 2021-06-21T02:49:11Z


 private:
  const AttributeMap& attrs_;
+  const AttributeMap* attrs_default_;


为什么不同样使用const &类型，这里有什么考虑吗

引用必须在构造函数的时候初始化。考虑到动态图静态图都使用AttrReader他的构造函数有2种：
AttrReader(const AttributeMap& attrs)
: attrs_(attrs), default_attrs_(nullptr) {}

AttrReader(const AttributeMap& attrs, const AttributeMap& default_attrs)
: attrs_(attrs), default_attrs_(&default_attrs) {}

chenwhql · 2021-06-21T02:58:04Z

  auto* attr_checker = op_info.Checker();
  if (attr_checker) {
-    attr_checker->Check(&attrs, true);
+    attr_checker->Check(&attrs, true, true);


建议这里加上参数名注释，方便找到是这里调用的，attr_checker->Check(&attrs, true, /*without_default_value=*/true);

chenwhql · 2021-06-21T02:59:29Z

-  void operator()(AttributeMap* attr_map,
-                  bool get_default_value_only = false) const {
+  void operator()(AttributeMap* attr_map, bool get_default_value_only = false,
+                  bool without_default_value = false) const {


without_default_value这个参数的命名理解起来比较困难，是不是改成only_check_exist_value之类的比较容易理解

without_default_value 让人有点儿容易和get_default_value_only 的作用混淆....

JiabinYang

some comments

JiabinYang · 2021-06-21T03:15:30Z

+  explicit AttrReader(const AttributeMap& attrs)
+      : attrs_(attrs), attrs_default_(nullptr) {}
+
+  AttrReader(const AttributeMap& attrs, const AttributeMap& attrs_default)


JiabinYang · 2021-06-21T03:17:08Z

-    PADDLE_ENFORCE_NE(attrs_.count(name), 0,
+    auto it = attrs_.find(name);
+    bool found = it != attrs_.end();
+    if (it == attrs_.end()) {


why not use found instead?

JiabinYang · 2021-06-21T03:18:25Z


 private:
  const AttributeMap& attrs_;
+  const AttributeMap* attrs_default_;


JiabinYang · 2021-06-21T03:21:29Z

-  void operator()(AttributeMap* attr_map,
-                  bool get_default_value_only = false) const {
+  void operator()(AttributeMap* attr_map, bool get_default_value_only = false,
+                  bool without_default_value = false) const {


without_default_value 让人有点儿容易和get_default_value_only 的作用混淆....

chenwhql

LGTM

JiabinYang

LGTM

raindrops2sea · 2021-06-22T14:11:50Z

    }
  }

  AttributeMap GetAttrsDefaultValuesMap() const {


GetDefaultAttrsValuesMap()？

raindrops2sea · 2021-06-22T14:13:41Z

    {
      imperative::TracedGradOp traced_grad_op(node);
      try {
+        traced_grad_op.SetAttrDefaultMap(this->DefaultAttrsMap());


SetDefaultAttrMap？

raindrops2sea · 2021-06-22T14:14:29Z

    return op_->SetAttrMap(attrs);
  }

+  void SetAttrDefaultMap(const framework::AttributeMap& attrs) {


SetDefaultAttrMap？

raindrops2sea · 2021-06-22T14:15:07Z

  DygraphInferShapeContext(const NameVarMap<VarType>* in,
                           const NameVarMap<VarType>* out,
                           const framework::AttributeMap* attr,
+                           const framework::AttributeMap* attr_default,


JiabinYang

LGTM

raindrops2sea

LGTM

optimize attr default value, test=develop

dd3329d

wanghuancoder added 8 commits June 4, 2021 10:37

refine, test=develop

c8b557b

refine, test=develop

8b1cdab

refine, test=develop

0cf6374

fix bug in AttrReader, test=develop

2facfca

fix bug, test=develop

911300c

fix double_grad, test=develop

3598f3c

refine, test=develop

2ff8504

refine, test=develop

e22ea5f

phlrain reviewed Jun 8, 2021

View reviewed changes

fix checker null, test=develop

d44aeb6

wanghuancoder requested a review from phlrain June 9, 2021 05:59

phlrain reviewed Jun 18, 2021

View reviewed changes

wanghuancoder added 6 commits June 18, 2021 08:43

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

0132959

… attr2

for test, test=develop

32e2f60

refine, test=develop

eb9b97e

refine, test=develop

e681acd

refine, test=develop

ea470d7

refine, test=develop

f54c359

chenwhql reviewed Jun 21, 2021

View reviewed changes

JiabinYang reviewed Jun 21, 2021

View reviewed changes

refine, test=develop

2d4c9dc

chenwhql previously approved these changes Jun 21, 2021

View reviewed changes

phlrain self-requested a review June 21, 2021 12:21

phlrain previously approved these changes Jun 21, 2021

View reviewed changes

JiabinYang previously approved these changes Jun 22, 2021

View reviewed changes

raindrops2sea reviewed Jun 22, 2021

View reviewed changes

refine, test=develop

806564c

wanghuancoder dismissed stale reviews from JiabinYang, phlrain, and chenwhql via 806564c June 23, 2021 01:25

JiabinYang approved these changes Jun 23, 2021

View reviewed changes

chenwhql approved these changes Jun 23, 2021

View reviewed changes

phlrain self-requested a review June 23, 2021 08:07

phlrain approved these changes Jun 23, 2021

View reviewed changes

raindrops2sea approved these changes Jun 23, 2021

View reviewed changes

wanghuancoder merged commit 5d2eb67 into PaddlePaddle:develop Jun 23, 2021

wanghuancoder mentioned this pull request Aug 3, 2021

fix attr can not find in mkldnn #34567

Merged

Conversation

wanghuancoder commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR types

PR changes

Describe

1. 背景

2. 内容

3. 性能测试

Uh oh!

paddle-bot-old Bot commented Jun 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paddle-bot-old Bot commented Jun 16, 2021

Uh oh!

phlrain Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenwhql left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wanghuancoder Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wanghuancoder commented Jun 4, 2021 •

edited

Loading

phlrain Jun 18, 2021 •

edited

Loading

wanghuancoder Jun 21, 2021 •

edited

Loading