zhztheplayer commented on code in PR #11493:
URL:
https://github.com/apache/incubator-gluten/pull/11493#discussion_r2731252785
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -327,22 +330,17 @@ int64_t
shrinkVeloxMemoryPool(velox::memory::MemoryManager* mm, velox::memory::M
std::shared_ptr<arrow::MemoryPool>
VeloxMemoryManager::getOrCreateArrowMemoryPool(const std::string& name) {
std::lock_guard<std::mutex> l(mutex_);
if (const auto it = arrowPools_.find(name); it != arrowPools_.end()) {
- auto pool = it->second.lock();
- VELOX_CHECK_NOT_NULL(pool, "Arrow memory pool {} has been destructed",
name);
- return pool;
+ if (auto pool = it->second.lock()) {
+ return pool;
+ }
+ arrowPools_.erase(name);
}
- auto pool = std::make_shared<ArrowMemoryPool>(
- blockListener_.get(), [this, name](arrow::MemoryPool* pool) {
this->dropMemoryPool(name); });
+
+ auto pool = std::make_shared<ArrowMemoryPool>(blockListener_.get());
Review Comment:
> It looks like not an issue as long as the number of keys is not too large.
I thought it might be up to the usage. Hi @marin-ma, do you think we can
skip cleaning up the Arrow pools? In that case, we can simply store shared
pointer as the class member.
> If necessary, perhaps looping over the weak_ptr each time
getOrCreateArrowMemoryPool is called and remove the expired ones.
Otherwise, this might be helpful, but we should also figure out whether it
will cause any instability regarding performance.
--
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]
