Refactorize Scope #3116
Refactorize Scope #3116
Conversation
|
|
| } | ||
|
|
||
| Variable* Scope::NewVar() { | ||
| return NewVar(string::Sprintf("%p.%d", this, vars_.size())); |
There was a problem hiding this comment.
It seems use vars_.size() as the unique key is not so good? why not atmoic or timestamp?
There was a problem hiding this comment.
I think scope address + the offset of the variable in this scope would be enough to uniquely identify the variable, isn't it?
| @@ -107,11 +107,11 @@ class Scope { | |||
| ``` | |||
| ## Only scope can create a variable | |||
|
|
|||
| To ensure `only scope can create a variable`, we should mark `Variable`'s constructor as a private member function, and Scope is a friend class of Variable. And then only `CreateVariable` can construct `Variable`. | |||
| To ensure `only scope can create a variable`, we should mark `Variable`'s constructor as a private member function, and Scope is a friend class of Variable. And then only `NewVar` can construct `Variable`. | |||
There was a problem hiding this comment.
use a macro like TF_DISALLOW_COPY_AND_ASSIGN better? https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/platform/macros.h#L84
There was a problem hiding this comment.
not only Variable, but alsoOperator, Tensor andScope should all be DISALLOW_COPY_AND_ASSIGN, this macro will make default and copy constructor = delete.
paddle/framework/scope.h
Outdated
| explicit Scope(Scope* parent) : parent_(parent) {} | ||
|
|
||
| std::map<std::string, Variable*> vars_; | ||
| std::list<Scope*> kids_; |
There was a problem hiding this comment.
why should we store kid scope and how can we use them?
In this PR, it seems that we will have a global default scope and all other scopes will be stored and deleted by this Scope if itself is deleted.
There was a problem hiding this comment.
You are right. The purpose of addingkids_ is to manage the lifecycle of Scopes.
* Make interface of Operator to `const Scope&`
* Automatically set use_dynamic_shape
Fixes #3113
Fixes #3114
Fixes #3115
Fixes #3117