Add Tensor::CopyFrom and Tensor::mutable_data(Place place) #2825

JiayiFeng · 2017-07-12T10:23:40Z

Add Tensor::CopyFrom. Current version can only support CPU memory
copy. The support of GPU will be provided later by paddle::memory.
The current implementation of Tensor::CopyFrom is a little inefficient:
Every time CopyFrom is called, tensor will re-allocate its memory. However, if
we try to check and reuse placeholder_, we have to provide a template
parameter for CopyFrom to indicate the data type. It seems strange for
a simple copy function.
Add Tensor::mutable_data(Place place), which directly use member
variable dims_ as its dim parameter. This interface is required by
Op::InferShape.

1. Add `Tensor::CopyFrom`. Current version can only support CPU memory copy. The support of GPU will be provided later by `paddle::memory`. The current implementation of `Tensor::CopyFrom` is a little inefficient: Every time `CopyFrom` is called, tensor will re-allocate its memory. However, if we try to check and reuse `placeholder_`, we have to provide a template parameter for `CopyFrom` to indicate the data type. It seems strange for a simple copy function. 2. Add `Tensor::mutable_data(Place place)`, which directly use member variable `dims_` as its dim parameter. This interface is required by `Op::InferShape`.

luotao1 · 2017-07-13T01:58:11Z

paddle/framework/tensor_test.cc

+  }
+
+  Tensor slice_tensor = src_tensor.Slice(1, 2);
+  dst_tensor.CopyFrom(slice_tensor, CPUPlace());


这个CopyFrom和memcpy还是不大一样吧。比如memcpy(src_ptr, att + 1, 8 *sizeof(int))，CopyFrom能做么？是准备后续增加？

我的理解是，这种可以随意指定开头和长度的拷贝对于tensor来说并不一定有意义，因为很可能拷贝出来的内容无法组成一个tensor，所以并不是CopyFrom应该提供的功能

Got it. 同意

qingqing01 · 2017-07-13T11:47:10Z

paddle/framework/tensor.h

-        || holder_->Size() < product(dims) * sizeof(T) + offset_) {
-      holder_.reset(new PlaceholderImpl<T>(place, product(dims) * sizeof(T)));
+        || holder_->Size() < product(dims_) * sizeof(T) + offset_) {
+      holder_.reset(new PlaceholderImpl<T>(place, product(dims_) * sizeof(T)));


Tensor() : offset_(0) {}

这个构造函数没有初始化dims_，假如先调用这个空的构造函数，再调用该mutable_data会有问题。

确实会有这样的问题……看来需要在mutable_data(Place place)中PADDLE_ENFORCE一下：

PADDLE_ENFORCE(product(dims_) > 0)

即使不提供mutable_data(Place place)接口，这个判断应该也是有必要的，因为在调用mutable_data(DDim dims, Place place)的时候也可能传一个{0}的DDim进来。

qingqing01 · 2017-07-13T11:49:24Z

paddle/framework/tensor.h

@@ -63,6 +70,15 @@ class Tensor {
    offset_ = src.offset_;
  }

+  void CopyFrom(const Tensor& src, paddle::platform::Place dst_place) {


dst_place和place_可能不一样？为什么要加dst_place？

确实可能不一样，可能跨设备拷贝。

qingqing01 · 2017-07-13T11:52:27Z

paddle/framework/tensor.h

+  void CopyFrom(const Tensor& src, paddle::platform::Place dst_place) {
+    PADDLE_ENFORCE(src.holder_ != nullptr,
+                   "Can not copy from an uninitialized tensor.");
+    size_t size = product(src.dims()) * src.holder_->TypeSize();


很多地方用了product()，觉得加一个获取size的接口也可以~

确实。另外我怀疑product可能存在性能问题，因为目前的实现是把DDim转化为vector<int>，再做连乘，可能会比较慢

同意获取size的接口。另外，能加个reshape dim的接口么，比如把dim<1, 10, 20>变成<10, 20>。

qingqing01 · 2017-07-13T11:54:11Z

paddle/framework/tensor.h

+  void CopyFrom(const Tensor& src, paddle::platform::Place dst_place) {
+    PADDLE_ENFORCE(src.holder_ != nullptr,
+                   "Can not copy from an uninitialized tensor.");
+    size_t size = product(src.dims()) * src.holder_->TypeSize();


* src.holder_->TypeSize() 也可以在PlaceholderImpl里吧

这个没太理解？

… dev_add_tensor_copy

1. Add template T which indicates data type to `CopyFrom()`, `Slice()` and `ShareData()` functions. This makes `CopyData()` code much clearer. 2. Add `set_dim()`. 3. `product(DDim)` transforms `DDim` to `vector<int>` first and then calculate its product. That might be quite slow. For `product(dims_)` is frequently used in Tensor, we add a mumber variable `numel_` as a cache of the product result. TODO: refactor `product()` to make it more efficient. 4. Unable Tensor::operator= 5. Remove the limit of POD type, because `float16` and `int8` are not POD type.

QiJune · 2017-07-14T11:16:48Z

paddle/framework/tensor.h

+    }
+    dims_ = dims;
+    numel_ = product(dims_);
+    return;


return here is unnecessary here, Please remove it

QiJune · 2017-07-14T11:20:37Z

paddle/framework/tensor.h

+  inline void CheckDimsValidity() const {
+    PADDLE_ENFORCE(holder_ != nullptr,
+                   "Tenosr holds no memory. Call Tensor::mutable_data first.");
+    PADDLE_ENFORCE(holder_->size() > numel_ * sizeof(T) + offset_,


>= will be good.
And maybe CheckDims is more concise than CheckDimsValidity

QiJune · 2017-07-14T11:24:18Z

paddle/framework/tensor.h

+    src.CheckDimsValidity<T>();
+    size_t size = src.numel_ * sizeof(T);
+    set_dims(src.dims());
+    const void* src_ptr = static_cast<const void*>(src.data<T>());


why use const here(static_cast<const void*>)? data() will return a const T*

const T* is not allowed to transform to void* by static_cast.

QiJune · 2017-07-14T11:38:54Z

paddle/framework/tensor.h

+  T* mutable_data(paddle::platform::Place place) {
+    PADDLE_ENFORCE(numel_ > 0,
+                   "Tensor::numel_ must be larger than zero to call "
+                   "Tensor::mutable_data.");


Users can not call this method, only he calls set_dims or mutable_data(dim,place) first. So, we should inform the users more clearly.

And is mutable_data(dim, place) necessary? It's just the combination of set_dims and mutable_data(place).

QiJune

LGTM

* fix sync_bn in cpu

JiayiFeng requested review from reyoung, a user, luotao1, qingqing01 and QiJune July 12, 2017 10:23

luotao1 reviewed Jul 13, 2017

View reviewed changes

qingqing01 reviewed Jul 13, 2017

View reviewed changes

JiayiFeng added 5 commits July 14, 2017 10:40

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

dfa4650

… dev_add_tensor_copy

Refactor Tensor::CopyFrom()

a1dc431

fix several compile error

1f97388

change int numel_ to size_t numel

8594d5c

QiJune reviewed Jul 14, 2017

View reviewed changes

update tensor.h

34beec0

QiJune reviewed Jul 14, 2017

View reviewed changes

update PADDLE_ENFORCE message

57a22db

QiJune approved these changes Jul 14, 2017

View reviewed changes

JiayiFeng merged commit c48fc4d into PaddlePaddle:develop Jul 14, 2017

JiayiFeng deleted the dev_add_tensor_copy branch July 14, 2017 12:44

heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021

fix sync_bn in cpu (PaddlePaddle#2825)

03f79b0

* fix sync_bn in cpu

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tensor::CopyFrom and Tensor::mutable_data(Place place) #2825

Add Tensor::CopyFrom and Tensor::mutable_data(Place place) #2825

JiayiFeng commented Jul 12, 2017

luotao1 Jul 13, 2017

JiayiFeng Jul 13, 2017 •

edited

Loading

luotao1 Jul 13, 2017

qingqing01 Jul 13, 2017

JiayiFeng Jul 13, 2017

qingqing01 Jul 13, 2017

JiayiFeng Jul 13, 2017

qingqing01 Jul 13, 2017

JiayiFeng Jul 13, 2017

luotao1 Jul 14, 2017

qingqing01 Jul 13, 2017

JiayiFeng Jul 13, 2017

QiJune Jul 14, 2017 •

edited

Loading

JiayiFeng Jul 14, 2017

QiJune Jul 14, 2017

JiayiFeng Jul 14, 2017

QiJune Jul 14, 2017

JiayiFeng Jul 14, 2017

QiJune Jul 14, 2017

JiayiFeng Jul 14, 2017

QiJune left a comment

Add Tensor::CopyFrom and Tensor::mutable_data(Place place) #2825

Add Tensor::CopyFrom and Tensor::mutable_data(Place place) #2825

Conversation

JiayiFeng commented Jul 12, 2017

Choose a reason for hiding this comment

JiayiFeng Jul 13, 2017 • edited Loading

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

QiJune Jul 14, 2017 • edited Loading

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

QiJune left a comment

Choose a reason for hiding this comment

JiayiFeng Jul 13, 2017 •

edited

Loading

QiJune Jul 14, 2017 •

edited

Loading