[Hexagon] Refactor HexagonBufferManager #13109

adstraw · 2022-10-17T22:04:37Z

Refactor HexagonBufferManager to handle post release buffer frees as a no-op using internal data structures versus returning state through the prior current_allocations method.

tvm-bot · 2022-10-17T22:04:40Z

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

cc @mehrdadh _{See #10317 for details}

_{Generated by tvm-bot}

adstraw · 2022-10-17T22:05:17Z

CC @janetsc and @Lunderberg RE post-review feedback on PR #13028

adstraw · 2022-10-17T22:43:03Z

@tvm-bot rerun

Lunderberg

I like the general cleanup, though it looks like there's some potential threading issues that could occur.

src/runtime/hexagon/hexagon_buffer_manager.h

Lunderberg · 2022-10-18T13:46:52Z

src/runtime/hexagon/hexagon_buffer_manager.h

+  void Reset() {
+    std::lock_guard<std::mutex> lock(mutex_);
+    hexagon_buffer_map_.clear();
+    released_buffers_.clear();


Should the released_buffers_ be cleared here? As I understand it, maintaining two separate lists is intended to avoid throwing an error in cases where the following sequence occurs.

Scope A enters

Allocation of X

Scope A exits, implicit de-allocation of X

Scope B enters

Request to de-allocate X.

If step 4 calls Reset and clears released_buffers_, then step 5 would raise an error.

Would like to hear thoughts from @janetsc. My understanding was that once we call AcquireResources in the Hexagon Device API (step 4) which in turn calls this Reset method we want the any past free of buffers (step 5) to fail.

Yes - once we call AcquireResources, we are starting a new session. Everything we had previously tracked should be forgotten.

OK, this is now handled with a Enable / Disable method. We check on Enable which is called from AcquireResources that we have freed everything.

Piggybacking of @Lunderberg's example, this means that we will fail in step 4 if we have not de-allocated X prior.

Lunderberg · 2022-10-18T13:51:41Z

src/runtime/hexagon/hexagon_buffer_manager.h

+    if (it_released != released_buffers_.end()) {
+      released_buffers_.erase(it_released);
+      return;
+    }


What do we want to happen if a void* occurs in both released_buffers_ and hexagon_buffer_map_? I'm picturing a case where the an allocation returns the same data pointer as a previous allocation that had been left open. I think with the current implementation, it would preferentially treat it as a known-previously-released buffer, and would not de-allocate the memory would be ignored, because released_buffers_ gets checked first. I don't think that's the desired behavior, because that could result in an unexpectedly higher memory footprint.

I think what we want here is to not allow any new allocations when released_buffers_ has any values.

In the current checked in code, that's what would happen because there would be no runtime_hexbuffs

Good point. I added a change to AllocateHexagonBuffer to remove ptr from the list of released_buffers_ so that it should only be in one data structure at a time.

There is no longer any released_buffers_ structure so the original feedback is no longer valid.

I think what we want here is to not allow any new allocations when released_buffers_ has any values.

This PR will fail / assert if we Enable the HexagonBufferManager with unfreed buffers.

janetsc

I totally agree that the currently implementation "breaks" some abstraction by allowing the list of allocated buffers to be saved before it is destroyed in ReleaseResources.

However, this introduces some new problems, I think, including the overall state of the device resources. By previously having a buffer management object created/destroyed in Acquire/Release, it allows is to manage resources very explicitly in a session. Now that the object lives in this PR, it has an additional state - "released," where no new allocations should be allowed.

I almost think it would be better to go for the solution where we clean up any frees that are currently happening after ReleaseResources, and make it an error to call Free after ReleaseResources.

janetsc · 2022-10-18T16:04:51Z

src/runtime/hexagon/hexagon_device_api.cc

-                          << "HexagonDeviceAPI::FreeWorkspace outside of a session.  "
-                          << "Please call HexagonDeviceAPI::AcquireResources";
-  CHECK(runtime_hexbuffs->count(data) != 0)
+  CHECK(runtime_hexbuffs.count(data) != 0)


This has the side effect that it will fail for things that were released in Release(). It isn't as clear that's the case with this refactor.

I think this check is redundant to what's already done in the FreeHexagonBuffer method.

src/runtime/hexagon/hexagon_buffer_manager.h

janetsc · 2022-10-18T16:06:28Z

src/runtime/hexagon/hexagon_buffer_manager.h

+  void Reset() {
+    std::lock_guard<std::mutex> lock(mutex_);
+    hexagon_buffer_map_.clear();
+    released_buffers_.clear();


Yes - once we call AcquireResources, we are starting a new session. Everything we had previously tracked should be forgotten.

janetsc · 2022-10-18T16:07:50Z

src/runtime/hexagon/hexagon_buffer_manager.h

+    if (it_released != released_buffers_.end()) {
+      released_buffers_.erase(it_released);
+      return;
+    }


I think what we want here is to not allow any new allocations when released_buffers_ has any values.

In the current checked in code, that's what would happen because there would be no runtime_hexbuffs

janetsc · 2022-10-18T20:43:58Z

src/runtime/hexagon/hexagon_device_api.h

    CHECK(runtime_vtcm) << "runtime_vtcm was not created in AcquireResources";
    runtime_vtcm.reset();
+
+    runtime_hexbuffs.Disable();


runtime_vtcm needs to be released last - otherwise if there are still VTCM buffers, it will not be able to free them.

janetsc

This latest version does not clear the map (and free the buffers) on ReleaseResources. That is the most important functionality for the Acquire/Release feature.

[Hexagon] Refactor HexagonBufferManager; add Release API

846aa7f

Lunderberg requested changes Oct 18, 2022

View reviewed changes

janetsc reviewed Oct 18, 2022

View reviewed changes

adstraw added 2 commits October 18, 2022 12:41

use simple approach with a bool to track HexBuffMgr mode

91a8db1

cleanup on names and comments

7fefbc0

adstraw changed the title ~~[Hexagon] Refactor HexagonBufferManager; add Release API~~ [Hexagon] Refactor HexagonBufferManager Oct 18, 2022

janetsc reviewed Oct 18, 2022

View reviewed changes

areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022

adstraw closed this Oct 19, 2022

adstraw deleted the straw-hex-refactor-hexbuffmgr branch October 19, 2022 22:10

[Hexagon] Refactor HexagonBufferManager #13109

[Hexagon] Refactor HexagonBufferManager #13109

Uh oh!

Conversation

adstraw commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvm-bot commented Oct 17, 2022

Uh oh!

adstraw commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adstraw commented Oct 17, 2022

Uh oh!

Lunderberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janetsc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janetsc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adstraw commented Oct 17, 2022 •

edited

Loading

adstraw commented Oct 17, 2022 •

edited

Loading

janetsc left a comment •

edited

Loading