Lunderberg commented on code in PR #13109:
URL: https://github.com/apache/tvm/pull/13109#discussion_r998238905
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -82,28 +87,35 @@ class HexagonBufferManager {
//! \brief Returns whether the HexagonBufferManager has any allocations.
bool empty() {
- std::lock_guard<std::mutex> lock(map_mutex_);
+ std::lock_guard<std::mutex> lock(mutex_);
return hexagon_buffer_map_.empty();
}
- //! \brief Returns a vector of currently allocated pointers, owned by the
manager.
- // Note - this should only be used by the device API to keep track of what
- // was in the manager when HexagonDeviceAPI::ReleaseResources is called.
- std::vector<void*> current_allocations() {
- std::vector<void*> allocated;
- std::lock_guard<std::mutex> lock(map_mutex_);
+ void Reset() {
+ std::lock_guard<std::mutex> lock(mutex_);
+ hexagon_buffer_map_.clear();
+ released_buffers_.clear();
+ }
+
+ void Release() {
for (const auto& [data_ptr, buffer] : hexagon_buffer_map_) {
- allocated.push_back(data_ptr);
+ FreeHexagonBuffer(data_ptr);
+ {
+ std::lock_guard<std::mutex> lock(mutex_);
Review Comment:
~~This would cause a lot of unnecessary release/re-acquire of the lock,
since `FreeHexagonBuffer` immediately acquires the lock again. What if we had
a private member function that may only be called when the mutex is already
acquired? That way, `FreeHexagonBuffer` could acquire the lock, then
immediately call the private method, and `Release` could acquire the lock once
outside the loop while calling the private method within the loop.~~
Edit: I think there's a bigger issue, see below.
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -82,28 +87,35 @@ class HexagonBufferManager {
//! \brief Returns whether the HexagonBufferManager has any allocations.
bool empty() {
- std::lock_guard<std::mutex> lock(map_mutex_);
+ std::lock_guard<std::mutex> lock(mutex_);
return hexagon_buffer_map_.empty();
}
- //! \brief Returns a vector of currently allocated pointers, owned by the
manager.
- // Note - this should only be used by the device API to keep track of what
- // was in the manager when HexagonDeviceAPI::ReleaseResources is called.
- std::vector<void*> current_allocations() {
- std::vector<void*> allocated;
- std::lock_guard<std::mutex> lock(map_mutex_);
+ void Reset() {
+ std::lock_guard<std::mutex> lock(mutex_);
+ hexagon_buffer_map_.clear();
+ released_buffers_.clear();
Review Comment:
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.
1. Scope A enters
2. Allocation of X
3. Scope A exits, implicit de-allocation of X
4. Scope B enters
5. Request to de-allocate X.
If step 4 calls `Reset` and clears `released_buffers_`, then step 5 would
raise an error.
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -40,14 +40,19 @@ class HexagonBufferManager {
* \param ptr Address of the HexagonBuffer as returned by
`AllocateHexagonBuffer`.
*/
void FreeHexagonBuffer(void* ptr) {
+ std::lock_guard<std::mutex> lock(mutex_);
+ // Check the list of buffers that were still allocated at the time of
release. If this buffer
+ // is in that list, this is a no-op as it is being freed as part of
another object teardown.
+ auto it_released = std::find(released_buffers_.begin(),
released_buffers_.end(), ptr);
Review Comment:
C++17 allows variable initialization to occur in an if statement, separate
from the condition itself. Along with using member-function find by using
`unordered_set`, this would become more readable:
```c++
if(auto it = released_buffers_.find(ptr); it != released_buffers_.end())
```
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -82,28 +87,35 @@ class HexagonBufferManager {
//! \brief Returns whether the HexagonBufferManager has any allocations.
bool empty() {
- std::lock_guard<std::mutex> lock(map_mutex_);
+ std::lock_guard<std::mutex> lock(mutex_);
return hexagon_buffer_map_.empty();
}
- //! \brief Returns a vector of currently allocated pointers, owned by the
manager.
- // Note - this should only be used by the device API to keep track of what
- // was in the manager when HexagonDeviceAPI::ReleaseResources is called.
- std::vector<void*> current_allocations() {
- std::vector<void*> allocated;
- std::lock_guard<std::mutex> lock(map_mutex_);
+ void Reset() {
+ std::lock_guard<std::mutex> lock(mutex_);
+ hexagon_buffer_map_.clear();
+ released_buffers_.clear();
+ }
+
+ void Release() {
for (const auto& [data_ptr, buffer] : hexagon_buffer_map_) {
- allocated.push_back(data_ptr);
+ FreeHexagonBuffer(data_ptr);
+ {
+ std::lock_guard<std::mutex> lock(mutex_);
Review Comment:
Looking at it again, because `FreeHexagonBuffer` may modify
`hexagon_buffer_map_`, the loop iterator would be invalidated even if we aren't
pre-empted by another thread. When `unordered_map::erase` is called, [only the
iterator](https://en.cppreference.com/w/cpp/container/unordered_map#Iterator_invalidation)
to the erased element is invalidated. However, that is exactly the iterator
we're holding, and what we need to increment for the loop. The compiler isn't
required to diagnose invalidated iterators, so we're into undefined behavior at
that point.
I think the following implementation would work better, both to avoid lock
release/re-acquire, and would avoid modification of `hexagon_buffer_map_` while
iterating over it.
```c++
void Release() {
std::lock_guard<std::mutex> lock(mutex_);
for(const auto& [data_ptr, buffer] : hexagon_buffer_map_) {
released_buffers_.insert(data_ptr);
}
hexagon_buffer_map_.clear();
}
```
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -40,14 +40,19 @@ class HexagonBufferManager {
* \param ptr Address of the HexagonBuffer as returned by
`AllocateHexagonBuffer`.
*/
void FreeHexagonBuffer(void* ptr) {
+ std::lock_guard<std::mutex> lock(mutex_);
+ // Check the list of buffers that were still allocated at the time of
release. If this buffer
+ // is in that list, this is a no-op as it is being freed as part of
another object teardown.
+ auto it_released = std::find(released_buffers_.begin(),
released_buffers_.end(), ptr);
+ if (it_released != released_buffers_.end()) {
+ released_buffers_.erase(it_released);
+ return;
+ }
Review Comment:
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.
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -82,28 +87,35 @@ class HexagonBufferManager {
//! \brief Returns whether the HexagonBufferManager has any allocations.
bool empty() {
- std::lock_guard<std::mutex> lock(map_mutex_);
+ std::lock_guard<std::mutex> lock(mutex_);
return hexagon_buffer_map_.empty();
}
- //! \brief Returns a vector of currently allocated pointers, owned by the
manager.
- // Note - this should only be used by the device API to keep track of what
- // was in the manager when HexagonDeviceAPI::ReleaseResources is called.
- std::vector<void*> current_allocations() {
- std::vector<void*> allocated;
- std::lock_guard<std::mutex> lock(map_mutex_);
+ void Reset() {
+ std::lock_guard<std::mutex> lock(mutex_);
+ hexagon_buffer_map_.clear();
+ released_buffers_.clear();
+ }
+
+ void Release() {
for (const auto& [data_ptr, buffer] : hexagon_buffer_map_) {
- allocated.push_back(data_ptr);
+ FreeHexagonBuffer(data_ptr);
+ {
+ std::lock_guard<std::mutex> lock(mutex_);
+ released_buffers_.push_back(data_ptr);
+ }
}
- return allocated;
}
private:
//! \brief Contains the HexagonBuffer objects managed by this class.
std::unordered_map<void*, std::unique_ptr<HexagonBuffer>>
hexagon_buffer_map_;
+ //! \brief Contains pointers to HexagonBuffer objects that had not been
freed at time of release
+ std::vector<void*> released_buffers_;
Review Comment:
Since the ordering doesn't matter and we are only ever checking whether a
value is inside the container, `std::unordered_set<void*>` would probably be a
better type.
--
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]
