[GitHub] [tvm] areusch commented on a diff in pull request #12727: [Hexagon] [runtime] Improve runtime resource management

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

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


##########
tests/cpp-runtime/hexagon/hexagon_device_api_tests.cc:
##########
@@ -146,3 +146,21 @@ TEST_F(HexagonDeviceAPITest, DISABLED_alloc_free_diff_dev) {
   CHECK(buf != nullptr);
   EXPECT_THROW(hexapi->FreeDataSpace(cpu_dev, buf), InternalError);
 }
+
+// Alloc a non-runtime buffer
+// Alloc a runtime buffer
+// "Release" resources for runtime
+// Verify the runtime buffer cannot be freed, but the non-runtime buffer can
+// This test should be run last
+TEST_F(HexagonDeviceAPITest, z_leak_resources) {

Review Comment:
   just curious, what's "z_" mean here?



##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -88,14 +88,23 @@ def __enter__(self):
                     self._rpc_receive_buffer_size_bytes,
                 ],
             )
+            func = self._rpc.get_function("device_api.hexagon.acquire_resources")
+            func()
             return self
 
         except RuntimeError as exception:
             raise exception
 
     def __exit__(self, exc_type, exc_value, exc_traceback):
-        # close session to the tracker
-        del self._rpc
+        try:
+            func = self._rpc.get_function("device_api.hexagon.release_resources")
+            func()
+        except RuntimeError:
+            # do nothing, as we are shutting down

Review Comment:
   we could probably `_LOG.warn("Exception occurred while calling release_resources() during Session __exit__", exc_info=1)` here



##########
src/runtime/hexagon/hexagon_device_api.h:
##########
@@ -50,6 +50,25 @@ class HexagonDeviceAPI final : public DeviceAPI {
   //! \brief Destructor
   ~HexagonDeviceAPI() {}
 
+  //! \brief Ensures resource managers are in a good state for the runtime
+  void AcquireResources() {
+    if (runtime_hexbuffs) {
+      LOG(INFO) << "runtime_hexbuffs has already been created";

Review Comment:
   should this be an error, since it might be dirty now?



##########
src/runtime/hexagon/hexagon_device_api.h:
##########
@@ -138,7 +157,11 @@ class HexagonDeviceAPI final : public DeviceAPI {
   }
 
   //! \brief Manages underlying HexagonBuffer allocations
+  // runtime_hexbuffs is used for runtime allocations.  It is created
+  // with a call to AcquireResources, and destroyed on ReleaseResources.

Review Comment:
   could you add some explanation of why the extra, non-RAII behavior is taken here?



-- 
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: commits-unsubscribe@tvm.apache.org

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