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

Implement framework::Variable #2587

Merged
merged 4 commits into from
Jun 26, 2017

Conversation

wangkuiyi
Copy link
Collaborator

I am using a C++ technique called type-information hiding in this implementation.

Compared with this design, my implementation doesn't use boost::any and thus doesn't rely on boost. Actually, this design relies nothing other than #include <typeinfo>.

Compare with Caffe2's implementation, mine doesn't use self-defined type inference mechanism like caffe2::TypeMeta, which is defined in caffe2/core/typeid.{h,cc,test_cc}, and saved about 500 lines C++ code:

$ wc -l ~/work/caffe2/caffe2/core/typeid*
      56 /Users/yiwang/work/caffe2/caffe2/core/typeid.cc
     300 /Users/yiwang/work/caffe2/caffe2/core/typeid.h
     136 /Users/yiwang/work/caffe2/caffe2/core/typeid_test.cc


template <typename T>
T* GetMutable() {
if (holder_ != nullptr && typeid(T) == holder_->Type()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::type_index(typeid(T)) == type_index(holder_->Type())

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

return allocated;
}

Placeholder* holder_; // pointers to a PlaceholderImpl object indeed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not unique_ptr?

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. using unique_ptr

virtual const std::type_info& Type() const { return type_; }
virtual void* Ptr() const { return ptr_; }

T* ptr_ = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

just unique_ptr is enough? no need for deleter.

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

public:
template <typename T>
const T& Get() const {
return *static_cast<const T*>(holder_->Ptr());
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to check whether T is same as holder_->Type().

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

namespace paddle {
namespace framework {

class Variable {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the variable design, we want to add Reset(T* ptr) function, which can delete the hosted pointer and set a new ptr. This interface is useful when we want to reuse a Variable and do not need to new another empty Variable. Should we add this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had Reset but deleted it later, because it is out of the imagined usage in the design doc (README.md). I think we can add it at any time in the future if we believe we do need it.

PlaceholderImpl(T* ptr) : ptr_(ptr), type_(typeid(T)) {}

virtual const std::type_info& Type() const { return type_; }
virtual void* Ptr() const { return static_cast<void*>(ptr_.get()); }
Copy link
Contributor

Choose a reason for hiding this comment

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

We also want to use std::unique_ptr for boost::any and do not manage the delete method in the variable design at first. But it needs to use get() method when returning the hosted pointer. I'm not sure whether it is good by returning the value using get() method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since cuDNN and other BLAS libraries would anyway need a raw pointer from unique_ptr or other smart pointers, I think it is OK (and indeed a mandatory) to call get().


BTW, I guess you wanted to say "We had thought about using std::unique_ptr ..."? :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Thanks! :)

virtual void* Ptr() const { return static_cast<void*>(ptr_.get()); }

std::unique_ptr<T> ptr_;
const std::type_info& type_;
Copy link
Contributor

@qingqing01 qingqing01 Jun 24, 2017

Choose a reason for hiding this comment

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

There is a unique id for the different type in the Caffe2's TypeMeta. The id is also used for type check in run-time. And the serializer method is created base on this id.

Here, we store the std::type_info to make check in run-time. There is a hash_code function in std:: type_info. This is equivalent to the unique id in the TypeMeta. Just comment here.

Copy link
Contributor

@hedaoyuan hedaoyuan left a comment

Choose a reason for hiding this comment

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

LGTM.
This pr is better than our previous design. Since it does not depend on boost::any. And remove the destructor with unique_ptr. I think this is the smallest implementation of Variable.

@wangkuiyi wangkuiyi merged commit a405e60 into PaddlePaddle:develop Jun 26, 2017
This was referenced Jun 26, 2017
/*input2*/ v2.Get<Tensor>().data());
```

We see that a variable holds nothing until `Variable::GetMutable<Tensor>()` allocates a tensor and puts it in the variable. Similarly, a tensor gets its memory until `Tensor::mutable_data()`.
Copy link
Collaborator

@JiayiFeng JiayiFeng Jun 26, 2017

Choose a reason for hiding this comment

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

It seems that 'lazy allocation' is split into two parts: the part in Variable and the part in Tensor. I am wondering whether following design will be a little better:

In Tensor:

Tensor<Device, T, D> {
  // return an empty tensor with ptr_ pointed to nullptr
  Tensr();
  // return a tensor with newly allocated memory
  Tensor(Dim<D> size);
  // check whether ptr_ points to nullptr
  bool is_empty();
}

In Variable:
before GetMutable<Tensor>() is called, Variable is assigned with an empty Tenosr which is constructed by Tensor::Tensor(). When GetMutable<Tensor>() is called, it checks whether the tensor is empty, if so, construct a new tensor by Tensor::Tensor(Dim<D> size) and then replace the empty one with it.

The replacement costs little time because the empty tensor holds no memory.

The benefit of this design is that all implement of 'lazy allocation' is now limited in Variable::GetMutable<Tensor>(), and we have no need to think about it during tensor implementation. It is a little like Majel's approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think Variable can hold anything -- it doens't have to be a Tensor, it could be a Scope even. Thus we might not initialize a Variable with an empty tensor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, thanks!

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.

5 participants