areusch commented on code in PR #10967:
URL: https://github.com/apache/tvm/pull/10967#discussion_r855393554


##########
python/tvm/rpc/client.py:
##########
@@ -514,7 +517,10 @@ def connect(url, port, key="", session_timeout=0, 
session_constructor_args=None)
         session_constructor_args = session_constructor_args if 
session_constructor_args else []
         if not isinstance(session_constructor_args, (list, tuple)):
             raise TypeError("Expect the session constructor to be a list or 
tuple")
-        sess = _ffi_api.Connect(url, port, key, *session_constructor_args)
+        enable_logging = False

Review Comment:
   since you just use this one kwarg and you aren't passing them en masse 
somewhere else, i think better function signature would be `connect(..., 
enable_logging=False)` rather than **kwargs. then you don't need this logic.



##########
python/tvm/rpc/client.py:
##########
@@ -505,7 +508,7 @@ def connect(url, port, key="", session_timeout=0, 
session_constructor_args=None)
         client_via_proxy = rpc.connect(
             proxy_server_url, proxy_server_port, proxy_server_key,
             session_constructor_args=[
-                "rpc.Connect", internal_url, internal_port, internal_key])
+                "rpc.Connect", internal_url, internal_port, internal_key, 
enable_logging])

Review Comment:
   i think here you mean
   ```suggestion
                   "rpc.Connect", internal_url, internal_port, internal_key],
                   enable_logging=True)
   ```



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.

Review Comment:
   params don't match the args



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {

Review Comment:
   expand the function name (e.g. `LogValue`). also, can you choose a better 
name for `s` and document its expected usage in a docstring?



##########
src/runtime/minrpc/minrpc_interfaces.h:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_INTERFACES_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_INTERFACES_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief Return interface used in ExecInterface to generate and sent the 
responses.

Review Comment:
   could you add similar docstrings for the functions here?



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.
+   */
+  explicit MinRPCReturnsWithLog(ReturnInterface* next) : next_(next), 
logger_() {}
+
+  ~MinRPCReturnsWithLog() {}
+
+  void ReturnVoid();
+
+  void ReturnHandle(void* handle);
+
+  void ReturnException(const char* msg);
+
+  void ReturnPackedSeq(const TVMValue* arg_values, const int* type_codes, int 
num_args);
+
+  void ReturnCopyAck(uint64_t* num_bytes, uint8_t** data_ptr);
+
+  void ReturnLastTVMError();
+
+  void ThrowError(RPCServerStatus code, RPCCode info = RPCCode::kNone);
+
+  void ProcessValues(const TVMValue** values, const int** tcodes, int* 
num_args);
+
+  void ResetCurrHandleName(RPCCode code);

Review Comment:
   nit: expand Curr -> Current or just remove it.



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.
+   */
+  explicit MinRPCReturnsWithLog(ReturnInterface* next) : next_(next), 
logger_() {}
+
+  ~MinRPCReturnsWithLog() {}
+
+  void ReturnVoid();
+
+  void ReturnHandle(void* handle);
+
+  void ReturnException(const char* msg);
+
+  void ReturnPackedSeq(const TVMValue* arg_values, const int* type_codes, int 
num_args);
+
+  void ReturnCopyAck(uint64_t* num_bytes, uint8_t** data_ptr);
+
+  void ReturnLastTVMError();
+
+  void ThrowError(RPCServerStatus code, RPCCode info = RPCCode::kNone);
+
+  void ProcessValues(const TVMValue** values, const int** tcodes, int* 
num_args);
+
+  void ResetCurrHandleName(RPCCode code);
+
+  void UpdateCurrHandleName(const char* name);
+
+  void GetHandleName(void* handle);
+
+  void ReleaseHandleName(void* handle);
+
+  Logger* GetLogger() { return &logger_; }
+
+ private:
+  void RegisterHandleName(void* handle);
+
+  ReturnInterface* next_;
+  std::string curr_handle_name_;
+  std::unordered_map<void*, std::string> array_tracker_;

Review Comment:
   suggest a name like `handle_descriptions_` or `handles_to_names_`



##########
src/runtime/minrpc/minrpc_logger.cc:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "minrpc_logger.h"
+
+#include <string.h>
+#include <time.h>
+#include <tvm/runtime/c_runtime_api.h>
+#include <tvm/runtime/logging.h>
+
+#include <functional>
+#include <iostream>
+#include <sstream>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+void Logger::LogTVMValue(int tcode, TVMValue value) {
+  switch (tcode) {
+    case kDLInt:
+      LogVal<int64_t>("(int64)", value.v_int64);
+      break;
+    case kDLUInt:
+      LogVal<uint64_t>("(uint64)", value.v_int64);
+      break;
+    case kDLFloat: {
+      LogVal<float>("(float)", value.v_float64);
+      break;
+    }
+    case kTVMDataType: {
+      LogDLData("DLDataType(code,bits,lane)", &value.v_type);
+      break;
+    }
+    case kDLDevice: {
+      LogDLDevice("DLDevice(type,id)", &value.v_device);
+      break;
+    }
+    case kTVMPackedFuncHandle: {
+      LogVal<void*>("(PackedFuncHandle)", value.v_handle);
+      break;
+    }
+    case kTVMModuleHandle: {
+      LogVal<void*>("(ModuleHandle)", value.v_handle);
+      break;
+    }
+    case kTVMOpaqueHandle: {
+      LogVal<void*>("(OpaqueHandle)", value.v_handle);
+      break;
+    }
+    case kTVMDLTensorHandle: {
+      LogVal<void*>("(TensorHandle)", value.v_handle);
+      break;
+    }
+    case kTVMNDArrayHandle: {
+      LogVal<void*>("kTVMNDArrayHandle", value.v_handle);
+      break;
+    }
+    case kTVMNullptr:
+      LogString("Nullptr");
+      break;
+    case kTVMStr: {
+      LogString("\"");
+      LogString(value.v_str);
+      LogString("\"");
+      break;
+    }
+    case kTVMBytes: {
+      TVMByteArray* bytes = static_cast<TVMByteArray*>(value.v_handle);
+      int len = bytes->size;
+      LogVal<int64_t>("(Bytes) [size]: ", len);
+      if (PRINT_BYTES) {
+        LogString(", [Values]:");
+        LogString(" { ");
+        if (len > 0) {
+          LogVal<uint64_t>("", (uint8_t)bytes->data[0]);
+        }
+        for (int j = 1; j < len; j++) LogVal<uint64_t>(" - ", 
(uint8_t)bytes->data[j]);
+        LogString(" } ");
+      }
+      break;
+    }
+    default: {
+      LogString("ERROR-kUnknownTypeCode)");
+      break;
+    }
+  }
+  LogString("; ");
+}
+
+void Logger::OutputLog() {
+  LOG(INFO) << os_.str();
+  os_.str(std::string());
+}
+
+void MinRPCReturnsWithLog::ReturnVoid() {
+  next_->ReturnVoid();
+  logger_.LogString("-> ReturnVoid");
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnHandle(void* handle) {
+  next_->ReturnHandle(handle);
+  if (code_ == RPCCode::kGetGlobalFunc) {
+    RegisterHandleName(handle);
+  }
+  logger_.LogVal<void*>("-> ReturnHandle: ", handle);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnException(const char* msg) {
+  next_->ReturnException(msg);
+  logger_.LogString("-> Exception: ");
+  logger_.LogString(msg);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnPackedSeq(const TVMValue* arg_values, const 
int* type_codes,
+                                           int num_args) {
+  next_->ReturnPackedSeq(arg_values, type_codes, num_args);
+  ProcessValues(&arg_values, &type_codes, &num_args);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnCopyAck(uint64_t* num_bytes, uint8_t** 
data_ptr) {
+  next_->ReturnCopyAck(num_bytes, data_ptr);
+  logger_.LogVal<uint64_t>("-> CopyAck: ", *num_bytes);
+  logger_.LogVal<void*>(", ", reinterpret_cast<void*>(*data_ptr));

Review Comment:
   i think you can use static_cast<void*> here (and in general, try to use this 
instead of reinterpret_cast wherever possible)



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }

Review Comment:
   these two functions (LogString and LogStr) are confusingly-named--when would 
you use LogString vs LogStr?
   
   Based on the impl, i think the usage might be the same (print this string to 
the log)--so you could rely on c++ function overloading e.g.
   `void Log(const char* s) { os_ << s; }`
   and 
   `void Log(std::string s) { os_ << s; }`
   



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.
+   */
+  explicit MinRPCReturnsWithLog(ReturnInterface* next) : next_(next), 
logger_() {}

Review Comment:
   nit: remove `, logger_()` since it's implied



##########
src/runtime/minrpc/minrpc_logger.cc:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "minrpc_logger.h"
+
+#include <string.h>
+#include <time.h>
+#include <tvm/runtime/c_runtime_api.h>
+#include <tvm/runtime/logging.h>
+
+#include <functional>
+#include <iostream>
+#include <sstream>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+void Logger::LogTVMValue(int tcode, TVMValue value) {
+  switch (tcode) {
+    case kDLInt:
+      LogVal<int64_t>("(int64)", value.v_int64);
+      break;
+    case kDLUInt:
+      LogVal<uint64_t>("(uint64)", value.v_int64);
+      break;
+    case kDLFloat: {
+      LogVal<float>("(float)", value.v_float64);
+      break;
+    }
+    case kTVMDataType: {
+      LogDLData("DLDataType(code,bits,lane)", &value.v_type);
+      break;
+    }
+    case kDLDevice: {
+      LogDLDevice("DLDevice(type,id)", &value.v_device);
+      break;
+    }
+    case kTVMPackedFuncHandle: {
+      LogVal<void*>("(PackedFuncHandle)", value.v_handle);
+      break;
+    }
+    case kTVMModuleHandle: {
+      LogVal<void*>("(ModuleHandle)", value.v_handle);
+      break;
+    }
+    case kTVMOpaqueHandle: {
+      LogVal<void*>("(OpaqueHandle)", value.v_handle);
+      break;
+    }
+    case kTVMDLTensorHandle: {
+      LogVal<void*>("(TensorHandle)", value.v_handle);
+      break;
+    }
+    case kTVMNDArrayHandle: {
+      LogVal<void*>("kTVMNDArrayHandle", value.v_handle);
+      break;
+    }
+    case kTVMNullptr:
+      LogString("Nullptr");
+      break;
+    case kTVMStr: {
+      LogString("\"");
+      LogString(value.v_str);
+      LogString("\"");
+      break;
+    }
+    case kTVMBytes: {
+      TVMByteArray* bytes = static_cast<TVMByteArray*>(value.v_handle);
+      int len = bytes->size;
+      LogVal<int64_t>("(Bytes) [size]: ", len);
+      if (PRINT_BYTES) {
+        LogString(", [Values]:");
+        LogString(" { ");
+        if (len > 0) {
+          LogVal<uint64_t>("", (uint8_t)bytes->data[0]);
+        }
+        for (int j = 1; j < len; j++) LogVal<uint64_t>(" - ", 
(uint8_t)bytes->data[j]);
+        LogString(" } ");
+      }
+      break;
+    }
+    default: {
+      LogString("ERROR-kUnknownTypeCode)");
+      break;
+    }
+  }
+  LogString("; ");
+}
+
+void Logger::OutputLog() {
+  LOG(INFO) << os_.str();
+  os_.str(std::string());
+}
+
+void MinRPCReturnsWithLog::ReturnVoid() {
+  next_->ReturnVoid();
+  logger_.LogString("-> ReturnVoid");
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnHandle(void* handle) {
+  next_->ReturnHandle(handle);
+  if (code_ == RPCCode::kGetGlobalFunc) {
+    RegisterHandleName(handle);
+  }
+  logger_.LogVal<void*>("-> ReturnHandle: ", handle);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnException(const char* msg) {
+  next_->ReturnException(msg);
+  logger_.LogString("-> Exception: ");
+  logger_.LogString(msg);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnPackedSeq(const TVMValue* arg_values, const 
int* type_codes,
+                                           int num_args) {
+  next_->ReturnPackedSeq(arg_values, type_codes, num_args);
+  ProcessValues(&arg_values, &type_codes, &num_args);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnCopyAck(uint64_t* num_bytes, uint8_t** 
data_ptr) {
+  next_->ReturnCopyAck(num_bytes, data_ptr);
+  logger_.LogVal<uint64_t>("-> CopyAck: ", *num_bytes);
+  logger_.LogVal<void*>(", ", reinterpret_cast<void*>(*data_ptr));
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnLastTVMError() {
+  const char* err = TVMGetLastError();
+  ReturnException(err);
+}
+
+void MinRPCReturnsWithLog::ThrowError(RPCServerStatus code, RPCCode info) {
+  next_->ThrowError(code, info);
+  logger_.LogString("-> ERROR");
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ProcessValues(const TVMValue** values, const int** 
tcodes,
+                                         int* num_args) {
+  if (*tcodes != nullptr) {
+    logger_.LogString("-> [");
+    for (int i = 0; i < *num_args; ++i) {
+      logger_.LogTVMValue((*tcodes)[i], (*values)[i]);
+
+      if ((*tcodes)[i] == kTVMOpaqueHandle) {
+        RegisterHandleName((*values)[i].v_handle);
+      }
+    }
+    logger_.LogString("]");
+  }
+}
+
+void MinRPCReturnsWithLog::ResetCurrHandleName(RPCCode code) {
+  code_ = code;
+  logger_.LogString(RPCCodeToString(code));
+  logger_.LogString(", ");
+  curr_handle_name_.clear();
+}
+
+void MinRPCReturnsWithLog::UpdateCurrHandleName(const char* name) {
+  if (curr_handle_name_.length() != 0) {
+    curr_handle_name_.append("::");
+  }
+  curr_handle_name_.append(name);
+}
+
+void MinRPCReturnsWithLog::GetHandleName(void* handle) {
+  if (array_tracker_.find(handle) != array_tracker_.end()) {
+    curr_handle_name_.append(array_tracker_[handle]);
+    logger_.LogHandleName(curr_handle_name_);
+  }
+}
+
+void MinRPCReturnsWithLog::ReleaseHandleName(void* handle) {
+  if (array_tracker_.find(handle) != array_tracker_.end()) {
+    logger_.LogHandleName(array_tracker_[handle]);

Review Comment:
   wondering if we need to Log at the same time we register?



##########
src/runtime/minrpc/minrpc_logger.cc:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "minrpc_logger.h"
+
+#include <string.h>
+#include <time.h>
+#include <tvm/runtime/c_runtime_api.h>
+#include <tvm/runtime/logging.h>
+
+#include <functional>
+#include <iostream>
+#include <sstream>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+void Logger::LogTVMValue(int tcode, TVMValue value) {
+  switch (tcode) {
+    case kDLInt:
+      LogVal<int64_t>("(int64)", value.v_int64);
+      break;
+    case kDLUInt:
+      LogVal<uint64_t>("(uint64)", value.v_int64);
+      break;
+    case kDLFloat: {
+      LogVal<float>("(float)", value.v_float64);
+      break;
+    }
+    case kTVMDataType: {
+      LogDLData("DLDataType(code,bits,lane)", &value.v_type);
+      break;
+    }
+    case kDLDevice: {
+      LogDLDevice("DLDevice(type,id)", &value.v_device);
+      break;
+    }
+    case kTVMPackedFuncHandle: {
+      LogVal<void*>("(PackedFuncHandle)", value.v_handle);
+      break;
+    }
+    case kTVMModuleHandle: {
+      LogVal<void*>("(ModuleHandle)", value.v_handle);
+      break;
+    }
+    case kTVMOpaqueHandle: {
+      LogVal<void*>("(OpaqueHandle)", value.v_handle);
+      break;
+    }
+    case kTVMDLTensorHandle: {
+      LogVal<void*>("(TensorHandle)", value.v_handle);
+      break;
+    }
+    case kTVMNDArrayHandle: {
+      LogVal<void*>("kTVMNDArrayHandle", value.v_handle);
+      break;
+    }
+    case kTVMNullptr:
+      LogString("Nullptr");
+      break;
+    case kTVMStr: {
+      LogString("\"");
+      LogString(value.v_str);
+      LogString("\"");
+      break;
+    }
+    case kTVMBytes: {
+      TVMByteArray* bytes = static_cast<TVMByteArray*>(value.v_handle);
+      int len = bytes->size;
+      LogVal<int64_t>("(Bytes) [size]: ", len);
+      if (PRINT_BYTES) {
+        LogString(", [Values]:");
+        LogString(" { ");
+        if (len > 0) {
+          LogVal<uint64_t>("", (uint8_t)bytes->data[0]);
+        }
+        for (int j = 1; j < len; j++) LogVal<uint64_t>(" - ", 
(uint8_t)bytes->data[j]);
+        LogString(" } ");
+      }
+      break;
+    }
+    default: {
+      LogString("ERROR-kUnknownTypeCode)");
+      break;
+    }
+  }
+  LogString("; ");
+}
+
+void Logger::OutputLog() {
+  LOG(INFO) << os_.str();
+  os_.str(std::string());
+}
+
+void MinRPCReturnsWithLog::ReturnVoid() {
+  next_->ReturnVoid();
+  logger_.LogString("-> ReturnVoid");
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnHandle(void* handle) {
+  next_->ReturnHandle(handle);
+  if (code_ == RPCCode::kGetGlobalFunc) {
+    RegisterHandleName(handle);
+  }
+  logger_.LogVal<void*>("-> ReturnHandle: ", handle);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnException(const char* msg) {
+  next_->ReturnException(msg);
+  logger_.LogString("-> Exception: ");
+  logger_.LogString(msg);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnPackedSeq(const TVMValue* arg_values, const 
int* type_codes,
+                                           int num_args) {
+  next_->ReturnPackedSeq(arg_values, type_codes, num_args);
+  ProcessValues(&arg_values, &type_codes, &num_args);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnCopyAck(uint64_t* num_bytes, uint8_t** 
data_ptr) {
+  next_->ReturnCopyAck(num_bytes, data_ptr);
+  logger_.LogVal<uint64_t>("-> CopyAck: ", *num_bytes);
+  logger_.LogVal<void*>(", ", reinterpret_cast<void*>(*data_ptr));
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnLastTVMError() {
+  const char* err = TVMGetLastError();
+  ReturnException(err);
+}
+
+void MinRPCReturnsWithLog::ThrowError(RPCServerStatus code, RPCCode info) {
+  next_->ThrowError(code, info);
+  logger_.LogString("-> ERROR");

Review Comment:
   worth logging the code?



##########
src/runtime/minrpc/minrpc_interfaces.h:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_INTERFACES_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_INTERFACES_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief Return interface used in ExecInterface to generate and sent the 
responses.
+ */
+class ReturnInterface {
+ public:
+  virtual ~ReturnInterface() {}
+  virtual void ReturnVoid() = 0;
+  virtual void ReturnHandle(void* handle) = 0;
+  virtual void ReturnException(const char* msg) = 0;
+  virtual void ReturnPackedSeq(const TVMValue* arg_values, const int* 
type_codes, int num_args) = 0;
+  virtual void ReturnCopyAck(uint64_t* num_bytes, uint8_t** data_ptr) = 0;
+  virtual void ReturnLastTVMError() = 0;
+  virtual void ThrowError(RPCServerStatus code, RPCCode info = RPCCode::kNone) 
= 0;
+};
+
+/*!
+ * \brief Execute interface used in MinRPCServer to process different received 
commands
+ */
+class ExecInterface {
+ public:
+  virtual ~ExecInterface() {}
+  virtual void ExecInitServer(int* num_args) = 0;

Review Comment:
   since this is ExecInterface and calling this function implies execution, I'd 
suggest we could shorten the names by removing "Exec" prefix here



##########
src/runtime/minrpc/minrpc_interfaces.h:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_INTERFACES_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_INTERFACES_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief Return interface used in ExecInterface to generate and sent the 
responses.
+ */
+class ReturnInterface {

Review Comment:
   it's a little odd that this is called ReturnInterface but all the impl are 
called MinRPCFoo. Suggest naming this MinRPCReturnInterface to keep with the 
pattern, or moving this all inside `namespace minrpc` and dropping MinRPC 
prefix everywhere.



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.
+   */
+  explicit MinRPCReturnsWithLog(ReturnInterface* next) : next_(next), 
logger_() {}
+
+  ~MinRPCReturnsWithLog() {}
+
+  void ReturnVoid();
+
+  void ReturnHandle(void* handle);
+
+  void ReturnException(const char* msg);
+
+  void ReturnPackedSeq(const TVMValue* arg_values, const int* type_codes, int 
num_args);
+
+  void ReturnCopyAck(uint64_t* num_bytes, uint8_t** data_ptr);
+
+  void ReturnLastTVMError();
+
+  void ThrowError(RPCServerStatus code, RPCCode info = RPCCode::kNone);
+
+  void ProcessValues(const TVMValue** values, const int** tcodes, int* 
num_args);
+
+  void ResetCurrHandleName(RPCCode code);
+
+  void UpdateCurrHandleName(const char* name);
+
+  void GetHandleName(void* handle);
+
+  void ReleaseHandleName(void* handle);
+
+  Logger* GetLogger() { return &logger_; }
+
+ private:
+  void RegisterHandleName(void* handle);
+
+  ReturnInterface* next_;
+  std::string curr_handle_name_;
+  std::unordered_map<void*, std::string> array_tracker_;
+  RPCCode code_;
+  Logger logger_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCExecute object, that also logs the responses.
+ *
+ * \tparam RExecInterface* underlying MinRPCExecute that processes the packets.
+ */
+class MinRPCExecuteWithLog : public ExecInterface {
+ public:
+  explicit MinRPCExecuteWithLog(ExecInterface* next) : next_(next) {
+    ret_handler_ = 
reinterpret_cast<MinRPCReturnsWithLog*>(next_->GetReturnInterface());

Review Comment:
   since this is the logging version of the ExecInterface, it's reasonable that 
the constructor should explicitly take a `Logger*`. i think you could add that 
argument here and that would remove the need to reinterpret_cast here.



##########
src/runtime/minrpc/minrpc_logger.cc:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "minrpc_logger.h"
+
+#include <string.h>
+#include <time.h>
+#include <tvm/runtime/c_runtime_api.h>
+#include <tvm/runtime/logging.h>
+
+#include <functional>
+#include <iostream>
+#include <sstream>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+void Logger::LogTVMValue(int tcode, TVMValue value) {
+  switch (tcode) {
+    case kDLInt:
+      LogVal<int64_t>("(int64)", value.v_int64);
+      break;
+    case kDLUInt:
+      LogVal<uint64_t>("(uint64)", value.v_int64);
+      break;
+    case kDLFloat: {
+      LogVal<float>("(float)", value.v_float64);
+      break;
+    }
+    case kTVMDataType: {
+      LogDLData("DLDataType(code,bits,lane)", &value.v_type);
+      break;
+    }
+    case kDLDevice: {
+      LogDLDevice("DLDevice(type,id)", &value.v_device);
+      break;
+    }
+    case kTVMPackedFuncHandle: {
+      LogVal<void*>("(PackedFuncHandle)", value.v_handle);
+      break;
+    }
+    case kTVMModuleHandle: {
+      LogVal<void*>("(ModuleHandle)", value.v_handle);
+      break;
+    }
+    case kTVMOpaqueHandle: {
+      LogVal<void*>("(OpaqueHandle)", value.v_handle);
+      break;
+    }
+    case kTVMDLTensorHandle: {
+      LogVal<void*>("(TensorHandle)", value.v_handle);
+      break;
+    }
+    case kTVMNDArrayHandle: {
+      LogVal<void*>("kTVMNDArrayHandle", value.v_handle);
+      break;
+    }
+    case kTVMNullptr:
+      LogString("Nullptr");
+      break;
+    case kTVMStr: {
+      LogString("\"");
+      LogString(value.v_str);
+      LogString("\"");
+      break;
+    }
+    case kTVMBytes: {
+      TVMByteArray* bytes = static_cast<TVMByteArray*>(value.v_handle);
+      int len = bytes->size;
+      LogVal<int64_t>("(Bytes) [size]: ", len);
+      if (PRINT_BYTES) {
+        LogString(", [Values]:");
+        LogString(" { ");
+        if (len > 0) {
+          LogVal<uint64_t>("", (uint8_t)bytes->data[0]);
+        }
+        for (int j = 1; j < len; j++) LogVal<uint64_t>(" - ", 
(uint8_t)bytes->data[j]);
+        LogString(" } ");
+      }
+      break;
+    }
+    default: {
+      LogString("ERROR-kUnknownTypeCode)");
+      break;
+    }
+  }
+  LogString("; ");
+}
+
+void Logger::OutputLog() {
+  LOG(INFO) << os_.str();
+  os_.str(std::string());
+}
+
+void MinRPCReturnsWithLog::ReturnVoid() {
+  next_->ReturnVoid();
+  logger_.LogString("-> ReturnVoid");
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnHandle(void* handle) {
+  next_->ReturnHandle(handle);
+  if (code_ == RPCCode::kGetGlobalFunc) {
+    RegisterHandleName(handle);
+  }
+  logger_.LogVal<void*>("-> ReturnHandle: ", handle);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnException(const char* msg) {
+  next_->ReturnException(msg);
+  logger_.LogString("-> Exception: ");
+  logger_.LogString(msg);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnPackedSeq(const TVMValue* arg_values, const 
int* type_codes,
+                                           int num_args) {
+  next_->ReturnPackedSeq(arg_values, type_codes, num_args);
+  ProcessValues(&arg_values, &type_codes, &num_args);
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnCopyAck(uint64_t* num_bytes, uint8_t** 
data_ptr) {
+  next_->ReturnCopyAck(num_bytes, data_ptr);
+  logger_.LogVal<uint64_t>("-> CopyAck: ", *num_bytes);
+  logger_.LogVal<void*>(", ", reinterpret_cast<void*>(*data_ptr));
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ReturnLastTVMError() {
+  const char* err = TVMGetLastError();
+  ReturnException(err);
+}
+
+void MinRPCReturnsWithLog::ThrowError(RPCServerStatus code, RPCCode info) {
+  next_->ThrowError(code, info);
+  logger_.LogString("-> ERROR");
+  logger_.OutputLog();
+}
+
+void MinRPCReturnsWithLog::ProcessValues(const TVMValue** values, const int** 
tcodes,
+                                         int* num_args) {
+  if (*tcodes != nullptr) {
+    logger_.LogString("-> [");
+    for (int i = 0; i < *num_args; ++i) {
+      logger_.LogTVMValue((*tcodes)[i], (*values)[i]);
+
+      if ((*tcodes)[i] == kTVMOpaqueHandle) {
+        RegisterHandleName((*values)[i].v_handle);
+      }
+    }
+    logger_.LogString("]");
+  }
+}
+
+void MinRPCReturnsWithLog::ResetCurrHandleName(RPCCode code) {
+  code_ = code;
+  logger_.LogString(RPCCodeToString(code));
+  logger_.LogString(", ");
+  curr_handle_name_.clear();
+}
+
+void MinRPCReturnsWithLog::UpdateCurrHandleName(const char* name) {
+  if (curr_handle_name_.length() != 0) {
+    curr_handle_name_.append("::");
+  }
+  curr_handle_name_.append(name);
+}
+
+void MinRPCReturnsWithLog::GetHandleName(void* handle) {
+  if (array_tracker_.find(handle) != array_tracker_.end()) {
+    curr_handle_name_.append(array_tracker_[handle]);
+    logger_.LogHandleName(curr_handle_name_);
+  }
+}
+
+void MinRPCReturnsWithLog::ReleaseHandleName(void* handle) {
+  if (array_tracker_.find(handle) != array_tracker_.end()) {
+    logger_.LogHandleName(array_tracker_[handle]);
+    array_tracker_.erase(handle);
+  }
+}
+
+void MinRPCReturnsWithLog::RegisterHandleName(void* handle) {
+  array_tracker_[handle] = curr_handle_name_;
+}
+
+void MinRPCExecuteWithLog::ExecInitServer(int* num_args) {
+  SetRPCCode(RPCCode::kInitServer);
+  logger_->LogString("Init Server");
+  next_->ExecInitServer(num_args);
+}
+
+void MinRPCExecuteWithLog::ExecNormalCallFunc(uint64_t* call_handle, 
TVMValue** values,
+                                              int** tcodes, int* num_args) {
+  SetRPCCode(RPCCode::kCallFunc);
+  logger_->LogVal<void*>("call_handle: ", 
reinterpret_cast<void*>(*call_handle));
+  ret_handler_->GetHandleName(reinterpret_cast<void*>(*call_handle));
+  if (*num_args > 0) {
+    logger_->LogString(", ");
+  }
+  ProcessValues(values, tcodes, num_args);
+  next_->ExecNormalCallFunc(call_handle, values, tcodes, num_args);
+}
+
+void MinRPCExecuteWithLog::ExecCopyFromRemote(DLTensor** arr, uint64_t* 
num_bytes,
+                                              uint8_t** temp_data) {
+  SetRPCCode(RPCCode::kCopyFromRemote);
+  logger_->LogVal<void*>("data_handle: ", 
reinterpret_cast<void*>((*arr)->data));
+  logger_->LogDLDevice(", DLDevice(type,id):", &(*arr)->device);
+  logger_->LogVal<int64_t>(", ndim: ", (*arr)->ndim);
+  logger_->LogDLData(", DLDataType(code,bits,lane): ", &(*arr)->dtype);
+  logger_->LogVal<uint64_t>(", num_bytes:", *num_bytes);
+  next_->ExecCopyFromRemote(arr, num_bytes, temp_data);
+}
+
+int MinRPCExecuteWithLog::ExecCopyToRemote(DLTensor** arr, uint64_t* num_bytes,
+                                           uint8_t** data_ptr) {
+  SetRPCCode(RPCCode::kCopyToRemote);
+  logger_->LogVal<void*>("data_handle: ", 
reinterpret_cast<void*>((*arr)->data));
+  logger_->LogDLDevice(", DLDevice(type,id):", &(*arr)->device);
+  logger_->LogVal<int64_t>(", ndim: ", (*arr)->ndim);
+  logger_->LogDLData(", DLDataType(code,bits,lane): ", &(*arr)->dtype);
+  logger_->LogVal<uint64_t>(", byte_offset: ", (*arr)->byte_offset);
+  return next_->ExecCopyToRemote(arr, num_bytes, data_ptr);
+}
+
+void MinRPCExecuteWithLog::ExecSyscallFunc(RPCCode* code, TVMValue** values, 
int** tcodes,
+                                           int* num_args) {
+  SetRPCCode(*code);
+  if ((*code) == RPCCode::kFreeHandle) {
+    if (((*num_args) == 2) && ((*tcodes)[0] == kTVMOpaqueHandle) && 
((*tcodes)[1] == kDLInt)) {
+      logger_->LogVal<void*>("handle: ", 
reinterpret_cast<void*>((*values)[0].v_handle));
+      if ((*values)[1].v_int64 == kTVMModuleHandle ||
+          (*values)[1].v_int64 == kTVMPackedFuncHandle) {
+        
ret_handler_->ReleaseHandleName(reinterpret_cast<void*>((*values)[0].v_handle));
+      }
+    }
+  } else {
+    ProcessValues(values, tcodes, num_args);
+  }
+  next_->ExecSyscallFunc(code, values, tcodes, num_args);
+}
+
+void MinRPCExecuteWithLog::ThrowError(RPCServerStatus code, RPCCode info) {
+  logger_->LogString("-> Error\n");
+  next_->ThrowError(code, info);
+}
+
+void MinRPCExecuteWithLog::ProcessValues(TVMValue** values, int** tcodes, int* 
num_args) {
+  if (*tcodes != nullptr) {
+    logger_->LogString("[");
+    for (int i = 0; i < *num_args; ++i) {
+      logger_->LogTVMValue((*tcodes)[i], (*values)[i]);
+
+      if ((*tcodes)[i] == kTVMStr) {
+        if (strlen((*values)[i].v_str) > 0) {
+          ret_handler_->UpdateCurrHandleName((*values)[i].v_str);

Review Comment:
   i think this is specific to `ModuleGetFunction` and `GetGlobalFunc` and 
`CallFunc`, right?



##########
src/runtime/minrpc/minrpc_server_logging.h:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_SERVER_LOGGING_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_SERVER_LOGGING_H_
+
+#include "minrpc_logger.h"
+#include "minrpc_server.h"
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief A minimum RPC server that logs the received commands.
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ */
+template <typename TIOHandler>
+class MinRPCServerWithLog {
+ public:
+  explicit MinRPCServerWithLog(TIOHandler* io)
+      : ret_handler_(io),
+        ret_handler_WLog_(&ret_handler_),
+        exec_handler_(io, &ret_handler_WLog_),
+        exec_handler_WLog_(&exec_handler_),
+        next_(io, &exec_handler_WLog_) {}
+
+  bool ProcessOnePacket() { return next_.ProcessOnePacket(); }
+
+ private:
+  MinRPCReturns<TIOHandler> ret_handler_;
+  MinRPCExecute<TIOHandler> exec_handler_;
+  MinRPCReturnsWithLog ret_handler_WLog_;
+  MinRPCExecuteWithLog exec_handler_WLog_;
+  MinRPCServer<TIOHandler> next_;
+};
+
+/*!
+ * \brief A minimum RPC server that only logs the outgoing commands and 
received responses.
+ * (Does not process the packets or respond to them.)
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ */
+template <typename TIOHandler>
+class MinRPCSniffer {
+ public:
+  explicit MinRPCSniffer(TIOHandler* io)
+      : io_(io),
+        ret_handler_(io_),
+        ret_handler_WLog_(&ret_handler_),
+        exec_handler_(&ret_handler_WLog_),
+        exec_handler_WLog_(&exec_handler_),
+        next_(io_, &exec_handler_WLog_) {}
+
+  bool ProcessOnePacket() { return next_.ProcessOnePacket(); }
+
+  void ProcessOneResponse() {
+    RPCCode code;
+    uint64_t packet_len = 0;
+
+    Read(&packet_len);
+    if (packet_len == 0) OutputLog();
+    Read(&code);
+    switch (code) {
+      case RPCCode::kReturn: {
+        HandleReturn();
+        break;
+      }
+      case RPCCode::kException: {
+        ret_handler_WLog_.ReturnException("");
+        break;
+      }
+      default: {
+        OutputLog();
+        break;
+      }
+    }
+  }
+
+ private:
+  void HandleReturn() {
+    int32_t num_args;
+    int32_t tcode;
+
+    Read(&num_args);
+    if (num_args == 1) {
+      Read(&tcode);
+      if (tcode == kTVMNullptr) {
+        ret_handler_WLog_.ReturnVoid();
+        return;
+      }
+      if (tcode == kTVMOpaqueHandle) {
+        uint64_t handle;
+        Read(&handle);
+        ret_handler_WLog_.ReturnHandle(reinterpret_cast<void*>(handle));
+        return;
+      }
+    }
+    OutputLog();
+  }
+
+  void OutputLog() { ret_handler_WLog_.GetLogger()->OutputLog(); }
+
+  template <typename T>
+  void Read(T* data) {
+    static_assert(std::is_trivial<T>::value && 
std::is_standard_layout<T>::value,
+                  "need to be trival");
+    ReadRawBytes(data, sizeof(T));
+  }
+
+  void ReadRawBytes(void* data, size_t size) {
+    uint8_t* buf = reinterpret_cast<uint8_t*>(data);
+    size_t ndone = 0;
+    while (ndone < size) {
+      ssize_t ret = io_->PosixRead(buf, size - ndone);
+      if (ret <= 0) {
+        ret_handler_WLog_.GetLogger()->LogString("-> No Response Received.");
+        break;
+      }
+      ndone += ret;
+      buf += ret;
+    }
+  }
+
+  TIOHandler* io_;
+  MinRPCReturnsNoOp<TIOHandler> ret_handler_;
+  MinRPCReturnsWithLog ret_handler_WLog_;

Review Comment:
   nit: could you name this all lowercase per 
https://google.github.io/styleguide/cppguide.html#Variable_Names
   
   here and below



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console

Review Comment:
   could you add docstrings to all of the functions here?



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.
+   */
+  explicit MinRPCReturnsWithLog(ReturnInterface* next) : next_(next), 
logger_() {}
+
+  ~MinRPCReturnsWithLog() {}
+
+  void ReturnVoid();
+
+  void ReturnHandle(void* handle);
+
+  void ReturnException(const char* msg);
+
+  void ReturnPackedSeq(const TVMValue* arg_values, const int* type_codes, int 
num_args);
+
+  void ReturnCopyAck(uint64_t* num_bytes, uint8_t** data_ptr);
+
+  void ReturnLastTVMError();
+
+  void ThrowError(RPCServerStatus code, RPCCode info = RPCCode::kNone);
+
+  void ProcessValues(const TVMValue** values, const int** tcodes, int* 
num_args);
+
+  void ResetCurrHandleName(RPCCode code);
+
+  void UpdateCurrHandleName(const char* name);
+
+  void GetHandleName(void* handle);
+
+  void ReleaseHandleName(void* handle);
+
+  Logger* GetLogger() { return &logger_; }
+
+ private:
+  void RegisterHandleName(void* handle);
+
+  ReturnInterface* next_;
+  std::string curr_handle_name_;
+  std::unordered_map<void*, std::string> array_tracker_;
+  RPCCode code_;
+  Logger logger_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCExecute object, that also logs the responses.
+ *
+ * \tparam RExecInterface* underlying MinRPCExecute that processes the packets.

Review Comment:
   i don't see this type param, can you remove?



##########
src/runtime/minrpc/minrpc_logger.h:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+#define TVM_RUNTIME_MINRPC_MINRPC_LOGGER_H_
+
+#include <tvm/runtime/c_runtime_api.h>
+
+#include <functional>
+#include <sstream>
+#include <string>
+#include <unordered_map>
+
+#include "minrpc_interfaces.h"
+#include "rpc_reference.h"
+
+namespace tvm {
+namespace runtime {
+
+#define PRINT_BYTES false
+
+/*!
+ * \brief Generates a user readeable log on the console
+ */
+class Logger {
+ public:
+  Logger() {}
+
+  void LogString(const char* s) { os_ << s; }
+
+  void LogStr(std::string s) { os_ << s; }
+
+  template <typename T>
+  void LogVal(const char* s, T val) {
+    os_ << s << val;
+  }
+
+  void LogDLDevice(const char* s, DLDevice* dev) {
+    os_ << s << "(" << dev->device_type << "," << dev->device_id << ")";
+  }
+
+  void LogDLData(const char* s, DLDataType* data) {
+    os_ << s << "(" << (uint16_t)data->code << "," << (uint16_t)data->bits << 
"," << data->lanes
+        << ")";
+  }
+
+  void LogHandleName(std::string name) {
+    if (name.length() > 0) {
+      os_ << " <" << name.c_str() << ">";
+    }
+  }
+
+  void LogTVMValue(int tcode, TVMValue value);
+  void OutputLog();
+
+ private:
+  std::stringstream os_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCReturns object, that also logs the responses.
+ *
+ * \tparam ReturnInterface* underlying MinRPCReturns that generates the 
responses.
+ */
+class MinRPCReturnsWithLog : public ReturnInterface {
+ public:
+  /*!
+   * \brief Constructor.
+   * \param io The IO handler.
+   */
+  explicit MinRPCReturnsWithLog(ReturnInterface* next) : next_(next), 
logger_() {}
+
+  ~MinRPCReturnsWithLog() {}
+
+  void ReturnVoid();
+
+  void ReturnHandle(void* handle);
+
+  void ReturnException(const char* msg);
+
+  void ReturnPackedSeq(const TVMValue* arg_values, const int* type_codes, int 
num_args);
+
+  void ReturnCopyAck(uint64_t* num_bytes, uint8_t** data_ptr);
+
+  void ReturnLastTVMError();
+
+  void ThrowError(RPCServerStatus code, RPCCode info = RPCCode::kNone);
+
+  void ProcessValues(const TVMValue** values, const int** tcodes, int* 
num_args);
+
+  void ResetCurrHandleName(RPCCode code);
+
+  void UpdateCurrHandleName(const char* name);
+
+  void GetHandleName(void* handle);
+
+  void ReleaseHandleName(void* handle);
+
+  Logger* GetLogger() { return &logger_; }
+
+ private:
+  void RegisterHandleName(void* handle);
+
+  ReturnInterface* next_;
+  std::string curr_handle_name_;
+  std::unordered_map<void*, std::string> array_tracker_;
+  RPCCode code_;
+  Logger logger_;
+};
+
+/*!
+ * \brief A wrapper for a MinRPCExecute object, that also logs the responses.
+ *
+ * \tparam RExecInterface* underlying MinRPCExecute that processes the packets.
+ */
+class MinRPCExecuteWithLog : public ExecInterface {
+ public:
+  explicit MinRPCExecuteWithLog(ExecInterface* next) : next_(next) {
+    ret_handler_ = 
reinterpret_cast<MinRPCReturnsWithLog*>(next_->GetReturnInterface());
+    logger_ = ret_handler_->GetLogger();
+  }
+
+  ~MinRPCExecuteWithLog() {}
+
+  void ExecInitServer(int* num_args);
+
+  void ExecNormalCallFunc(uint64_t* call_handle, TVMValue** values, int** 
tcodes, int* num_args);

Review Comment:
   i think all of these args are pass-by-pointer when they don't need to be, 
right? 



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to