[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18570: Reducing the number of calls of the NDArray constructors

[GitBox <gi...@apache.org>]

leezu commented on a change in pull request #18570:
URL: https://github.com/apache/incubator-mxnet/pull/18570#discussion_r461778722



##########
File path: src/imperative/imperative_utils.h
##########
@@ -35,6 +35,59 @@
 #define MXNET_IMPERATIVE_IMPERATIVE_UTILS_H_
 
 namespace mxnet {
+
+#if MXNET_USE_MKLDNN == 1
+template<typename T>T *pntr(T &obj)           { return &obj; }  // NOLINT
+template<typename T>T *pntr(T *obj)           { return obj; }
+
+template<typename T>
+void InvalidateOutputs(const std::vector<T> *pArrs,
+                        const std::vector<OpReqType> &reqs) {
+  auto arrs = *pArrs;
+  for (size_t i = 0; i < arrs.size(); i++) {
+    if (reqs[i] == kWriteTo || reqs[i] == kNullOp)
+      pntr(arrs[i])->InvalidateMKLDNNData();
+  }
+}
+
+static inline void CreateDefaultInputs(const std::vector<NDArray> &arrs,
+                                       std::vector<NDArray> *out_arrs) {
+  out_arrs->clear();
+  for (size_t i = 0; i < arrs.size(); ++i) {
+    if (arrs[i].IsMKLDNNData())
+      out_arrs->push_back(arrs[i].Reorder2Default());
+    else
+      out_arrs->push_back(arrs[i]);
+  }
+}
+
+static inline void CreateDefaultInputs(std::vector<NDArray> *pArrs) {
+  auto &&arrs = *pArrs;
+  for (size_t i = 0; i < arrs.size(); ++i)
+    arrs[i].SelfReorder2Default();
+}
+
+#define INVALIDATE_OUTPUTS(outputs, req) InvalidateOutputs(&outputs, req)

Review comment:
       Thank you for simplifying the code. Do you think it would be better to rely on `constexpr if` inside the functions (`IsDefaultData`, `InvalidateOutputs`, ...) instead of adding more Macros to the codebase?

##########
File path: src/imperative/imperative_utils.h
##########
@@ -35,6 +35,59 @@
 #define MXNET_IMPERATIVE_IMPERATIVE_UTILS_H_
 
 namespace mxnet {
+
+#if MXNET_USE_MKLDNN == 1
+template<typename T>T *pntr(T &obj)           { return &obj; }  // NOLINT
+template<typename T>T *pntr(T *obj)           { return obj; }
+
+template<typename T>
+void InvalidateOutputs(const std::vector<T> *pArrs,
+                        const std::vector<OpReqType> &reqs) {
+  auto arrs = *pArrs;
+  for (size_t i = 0; i < arrs.size(); i++) {
+    if (reqs[i] == kWriteTo || reqs[i] == kNullOp)
+      pntr(arrs[i])->InvalidateMKLDNNData();
+  }
+}
+
+static inline void CreateDefaultInputs(const std::vector<NDArray> &arrs,
+                                       std::vector<NDArray> *out_arrs) {
+  out_arrs->clear();
+  for (size_t i = 0; i < arrs.size(); ++i) {
+    if (arrs[i].IsMKLDNNData())
+      out_arrs->push_back(arrs[i].Reorder2Default());
+    else
+      out_arrs->push_back(arrs[i]);
+  }
+}
+
+static inline void CreateDefaultInputs(std::vector<NDArray> *pArrs) {
+  auto &&arrs = *pArrs;
+  for (size_t i = 0; i < arrs.size(); ++i)
+    arrs[i].SelfReorder2Default();
+}

Review comment:
       > It looks like I lost this comment line when I was moving `static inline void CreateDefaultInputs` from one file into another. I just recreated it and rebaised this PR branch.
   
   Thank you. The subgraph feature is available now, so the TODO can be addressed (and preserving the comment can be helpful).
   
   > First of all, by replacing NDArray constructors followed by assign operators by some Init function we should see some run time improvement, just because in Init function we do exactly the same as it was done in the constructor, but not doing assigns AND not calling destructors after that.
   
   > But the main goal of this replacement was the simplification of the tracing the NDArray objects during the debugging process.
   
   Thank you for elaborating @andrei5055. Simplifying the tracing of NDArray objects by using `ReInit` is helpful. My point is that the NDArray already has a cheap default constructor which would be invoked by the declaration of `std::vector<NDArray> inputs, outputs;` in `PushOperator` etc.:
   
   https://github.com/apache/incubator-mxnet/blob/7908d7eb56fc9d20c12afffd8ea592b959b80bfc/include/mxnet/ndarray.h#L84-L87
   
   So why add a separate `Init` method with unclear semantics? It seems you're not using the `Init` method and the `ReInit` should be sufficient
   
   Also note that your claim "usually we call some NDArray constructor AND NDArray` copy constructor" may not apply due to mandatory elision of copy/move operations in C++17.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org