You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "marin-ma (via GitHub)" <gi...@apache.org> on 2026/01/26 14:11:03 UTC
[PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
marin-ma opened a new pull request, #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493
(no comment)
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "baibaichen (via GitHub)" <gi...@apache.org>.
baibaichen merged PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "baibaichen (via GitHub)" <gi...@apache.org>.
baibaichen commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2732572286
##########
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:
If there aren’t many entries in `arrowPools_`, I think it’s fine to keep them via `shared_ptr`. A `MemoryPool` object itself shouldn’t take much memory anyway.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2728802315
##########
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:
If we no longer remove the entries, will there be any memory leak risk caused by the expanding entry list in `arrowPools_`?
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#issuecomment-3812326117
@baibaichen Do you have any more comments? Thanks!
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2733238789
##########
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:
Using `weak_ptr` can help to clarify the ownership of the arrow pool. For example if the arrow pool is used by shuffle A, it should be destroyed together with the shuffle writer A itself. And then the next shuffle B will create a new pool. In this way we can also track the memory allocation status for each shuffle separately.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2731070417
##########
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:
@baibaichen mentioned in the issue that we can use a background thread to clean up, but I wonder if that might be too heavy for the current use of the arrow pool, since only shuffle and parquet write create named arrow memory pool.
If necessary, perhaps looping over the weak_ptr each time `getOrCreateArrowMemoryPool` is called and remove the expired ones. @zhztheplayer What do you think?
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2737247735
##########
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:
Because in the latest PR's code, the expired map entries won't get removed from map until being replaced by new value of the same key. Is this an intentional change?
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2737233863
##########
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:
@marin-ma I meant, is it feasible to use `std::unordered_map<std::string, std::shared_ptr<ArrowMemoryPool>> arrowPools_`? Only by changing the map value type from `std::weak_ptr` to `std::shared_ptr`.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2737427560
##########
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:
> Because in the latest PR's code, the expired map entries won't get removed from map until being replaced by new value of the same key. Is this an intentional change?
Yes. In this way it can create new arrow pools for each shuffle writer instance. If changing to `std::unordered_map<std::string, std::shared_ptr<ArrowMemoryPool>> arrowPools_`, the new shuffle writers will reuse the ones created by the previous shuffle.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "baibaichen (via GitHub)" <gi...@apache.org>.
baibaichen commented on code in PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#discussion_r2729972631
##########
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.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-11485][VL] Fix the race condition in ArrowMemoryPool [incubator-gluten]
Posted by "baibaichen (via GitHub)" <gi...@apache.org>.
baibaichen commented on PR #11493:
URL: https://github.com/apache/incubator-gluten/pull/11493#issuecomment-3814917388
> @baibaichen Do you have any more comments? Thanks!
No. Let's merge to see whether the issue is fixed or not.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org