zjuwangg commented on code in PR #10111:
URL:
https://github.com/apache/incubator-gluten/pull/10111#discussion_r2207288122
##########
cpp/core/utils/ObjectStore.cc:
##########
@@ -37,23 +37,34 @@ gluten::ResourceMap<gluten::ObjectStore*>&
gluten::ObjectStore::stores() {
}
gluten::ObjectStore::~ObjectStore() {
- // destructing in reversed order (the last added object destructed first)
- const std::lock_guard<std::mutex> lock(mtx_);
- for (auto itr = aliveObjects_.rbegin(); itr != aliveObjects_.rend(); itr++) {
- const ResourceHandle handle = (*itr).first;
- const auto& info = (*itr).second;
- const std::string_view typeName = info.typeName;
- const size_t size = info.size;
- VLOG(2) << "Unclosed object ["
- << "Store ID: " << storeId_ << ", Resource handle ID: " << handle
<< ", TypeName: " << typeName
- << ", Size: " << size
- << "] is found when object store is closing. Gluten will"
- " destroy it automatically but it's recommended to manually
close"
- " the object through the Java closing API after use,"
- " to minimize peak memory pressure of the application.";
- store_.erase(handle);
+ std::vector<std::shared_ptr<void>> objects;
+ {
+ // destructing in reversed order (the last added object destructed first)
+ const std::lock_guard<std::mutex> lock(mtx_);
+ for (auto itr = aliveObjects_.rbegin(); itr != aliveObjects_.rend();
itr++) {
+ const ResourceHandle handle = (*itr).first;
+ const auto& info = (*itr).second;
+ const std::string_view typeName = info.typeName;
+ const size_t size = info.size;
+ VLOG(2) << "Unclosed object ["
+ << "Store ID: " << storeId_ << ", Resource handle ID: " <<
handle << ", TypeName: " << typeName
+ << ", Size: " << size
+ << "] is found when object store is closing. Gluten will"
+ " destroy it automatically but it's recommended to manually
close"
+ " the object through the Java closing API after use,"
+ " to minimize peak memory pressure of the application.";
+ // transfer ownership of the object to the temporary vector
+ objects.push_back(store_.lookup(handle));
+ store_.erase(handle);
+ }
+ stores().erase(storeId_);
+ }
+ // destructing objects outside the lock to avoid deadlock
+ for (auto& obj : objects) {
+ if (obj) {
+ obj.reset();
Review Comment:
It should be okey, will address it tomorrow.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
