feature/analysis node representation #10522
Conversation
…/inference-framework
…/inference-framework
Superjomn
changed the title
| template <typename T> | ||
| class OrderedRegistry { | ||
| public: | ||
| T *Register(const std::string &name, T *x) { |
There was a problem hiding this comment.
can this be std::unique_ptr? if the ownership is transferred.
There was a problem hiding this comment.
Because the pointer will be used by others and a shared_ptr is a little heavy here, so the implementation of this function is
dic_[name] = data_.size();
data_.emplace_back(std::unique_ptr<T>(x));
return data_.back().get();Here just a Register and a Lookup, without a Remove API, so the owner can't be transferred to others.
There was a problem hiding this comment.
I'm not suggesting a shared_ptr here.
Usually, unique_ptr in function def means the ownership of the pointer is tranferred to the function.
T *Register(const std::string &name, std::unique_ptr<T> x)
Register("foo", std::move(some_unique_ptr))
| // Cast to a subclass type, Function for example. | ||
| template <typename Subclass> | ||
| Subclass &As() { | ||
| return *reinterpret_cast<Subclass *>(this); |
There was a problem hiding this comment.
dynamic_cast is safer here?
| namespace analysis { | ||
|
|
||
| std::vector<Dot::Attr> Value::dot_attrs() const { | ||
| return std::vector<Dot::Attr>({Dot::Attr("style", "filled,rounded"), |
There was a problem hiding this comment.
what are these "style" "shape" thing?
There was a problem hiding this comment.
These are DOT language's attributes, to control the visualization for graph debug.
Here, different Node might have the different visual effect, for example, Operator and Variable looks different.
| // DOT node representation. One Node type can customize its own node | ||
| // representation. | ||
| virtual std::vector<Dot::Attr> dot_attrs() const { | ||
| return std::vector<Dot::Attr>({Dot::Attr("style", "filled")}); |
There was a problem hiding this comment.
what is "style" "filled"?
There was a problem hiding this comment.
This DOT's syntax.
| data.resize(sizeof(T)); | ||
| } | ||
| PADDLE_ENFORCE_EQ(data.size(), sizeof(T), "Node attr type recast error"); | ||
| return *reinterpret_cast<T *>(&data[0]); |
There was a problem hiding this comment.
this feels crazy dangerous to me. You basically allow people to cast it to anything and then cause all kinds of memory issues?
| template <typename T> | ||
| class OrderedRegistry { | ||
| public: | ||
| T *Register(const std::string &name, T *x) { |
There was a problem hiding this comment.
I'm not suggesting a shared_ptr here.
Usually, unique_ptr in function def means the ownership of the pointer is tranferred to the function.
T *Register(const std::string &name, std::unique_ptr<T> x)
Register("foo", std::move(some_unique_ptr))
| namespace analysis { | ||
|
|
||
| std::vector<Dot::Attr> Value::dot_attrs() const { | ||
| return std::vector<Dot::Attr>({Dot::Attr("style", "filled,rounded"), |
| // DOT node representation. One Node type can customize its own node | ||
| // representation. | ||
| virtual std::vector<Dot::Attr> dot_attrs() const { | ||
| return std::vector<Dot::Attr>({Dot::Attr("style", "filled")}); |
| @@ -105,16 +105,19 @@ class Node { | |||
| template <typename T> | |||
| T &As() { | |||
There was a problem hiding this comment.
I suggest to split this into a few different methods:
AsFoo()
AsBar()
fixes: #10602