-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Implement framework::Variable #2587
Conversation
paddle/framework/variable.h
Outdated
|
||
template <typename T> | ||
T* GetMutable() { | ||
if (holder_ != nullptr && typeid(T) == holder_->Type()) { |
There was a problem hiding this comment.
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())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
paddle/framework/variable.h
Outdated
return allocated; | ||
} | ||
|
||
Placeholder* holder_; // pointers to a PlaceholderImpl object indeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not unique_ptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. using unique_ptr
paddle/framework/variable.h
Outdated
virtual const std::type_info& Type() const { return type_; } | ||
virtual void* Ptr() const { return ptr_; } | ||
|
||
T* ptr_ = nullptr; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
paddle/framework/variable.h
Outdated
public: | ||
template <typename T> | ||
const T& Get() const { | ||
return *static_cast<const T*>(holder_->Ptr()); |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()); } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ..."? :-)
There was a problem hiding this comment.
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_; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
.
/*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()`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks!
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: