liuneng1994 commented on code in PR #13367:
URL: https://github.com/apache/gluten/pull/13367#discussion_r4345085454
##########
cpp/bolt/memory/BoltGlutenMemoryManager.cc:
##########
@@ -233,50 +233,49 @@
bytedance::bolt::memory::sparksql::BoltMemoryManagerHolder* BoltGlutenMemoryMana
void BoltGlutenMemoryManager::destroy(int64_t taskId, int64_t
memoryManagerHandle) {
GLUTEN_CHECK(enabled_, "Expect BoltMemoryManager enabled");
- int64_t holderCount = 0, deleteCount = 0;
std::vector<bytedance::bolt::memory::sparksql::BoltMemoryManagerHolderKey>
toBeDeletedKey;
std::vector<bytedance::bolt::memory::sparksql::BoltMemoryManagerHolder*>
toBeDeletedHolders;
{
std::lock_guard<std::mutex> guard(lock_);
for (const auto& pair : holders_) {
- if (pair.first.taskId == taskId) {
- holderCount++;
- if (pair.first.memoryManagerHandle == memoryManagerHandle) {
- toBeDeletedKey.emplace_back(pair.first);
- toBeDeletedHolders.emplace_back(pair.second);
- }
+ if (pair.first.taskId == taskId && pair.first.memoryManagerHandle ==
memoryManagerHandle) {
+ toBeDeletedKey.emplace_back(pair.first);
+ toBeDeletedHolders.emplace_back(pair.second);
}
}
- deleteCount = toBeDeletedKey.size();
+ if (toBeDeletedHolders.empty()) {
+ return;
+ }
for (const auto& key : toBeDeletedKey) {
holders_.erase(key);
}
+ pendingDestructions_[taskId] += toBeDeletedHolders.size();
}
- // delete holder out of mutex because BoltMemoryManager destruction may wait
for memory release,
- // which will block all tasks' destroying in current executor
- for (auto hold : toBeDeletedHolders) {
- if (hold != nullptr) {
- delete hold;
- }
+ // Destruction may wait for allocations owned by asynchronous work, so do
not hold lock_.
+ for (auto holder : toBeDeletedHolders) {
+ delete holder;
Review Comment:
If `delete holder` throws, `pendingDestructions_[taskId]` is never
decremented, so the task's `tmmCache_` entry and `ExecutionMemoryPool`
accounting leak for the lifetime of the executor. Please wrap the deletion loop
in try/finally and always decrement.
##########
cpp/bolt/memory/BoltGlutenMemoryManager.cc:
##########
@@ -233,50 +233,49 @@
bytedance::bolt::memory::sparksql::BoltMemoryManagerHolder* BoltGlutenMemoryMana
void BoltGlutenMemoryManager::destroy(int64_t taskId, int64_t
memoryManagerHandle) {
GLUTEN_CHECK(enabled_, "Expect BoltMemoryManager enabled");
- int64_t holderCount = 0, deleteCount = 0;
std::vector<bytedance::bolt::memory::sparksql::BoltMemoryManagerHolderKey>
toBeDeletedKey;
std::vector<bytedance::bolt::memory::sparksql::BoltMemoryManagerHolder*>
toBeDeletedHolders;
{
std::lock_guard<std::mutex> guard(lock_);
for (const auto& pair : holders_) {
- if (pair.first.taskId == taskId) {
- holderCount++;
- if (pair.first.memoryManagerHandle == memoryManagerHandle) {
- toBeDeletedKey.emplace_back(pair.first);
- toBeDeletedHolders.emplace_back(pair.second);
- }
+ if (pair.first.taskId == taskId && pair.first.memoryManagerHandle ==
memoryManagerHandle) {
+ toBeDeletedKey.emplace_back(pair.first);
+ toBeDeletedHolders.emplace_back(pair.second);
}
}
- deleteCount = toBeDeletedKey.size();
+ if (toBeDeletedHolders.empty()) {
Review Comment:
This early return also skips the `tmmCache_` cleanup. That matches the old
behaviour when no holder existed, but with the new `taskId == -1` driver path
it is worth confirming there is no state where a `tmmCache_` entry exists
without a corresponding holder (e.g. a holder that failed to register). If
there is, the entry is never reclaimed.
##########
cpp/bolt/tests/CMakeLists.txt:
##########
@@ -73,7 +73,25 @@ add_bolt_test(bolt_memory_test SOURCES MemoryManagerTest.cc)
add_bolt_test(buffer_outputstream_test SOURCES BufferOutputStreamTest.cc)
if(BUILD_EXAMPLES)
- add_bolt_test(my_udf_test SOURCES MyUdfTest.cc)
+ # The UDF plugin and its test must share one engine, including its function
+ # and protobuf registries. Avoid the static engine in add_bolt_test().
+ add_executable(my_udf_test MyUdfTest.cc)
Review Comment:
Bypassing `add_bolt_test` also drops `BOLT_TEST_COMMON_SRCS`, the standard
include dirs, and the `bolt::bolt`/glog link set. That is presumably
intentional for the shared-engine requirement, but
`$<TARGET_FILE:${PROJECT_NAME}>` now requires the shared library to exist;
please guard/assert for `BUILD_STATIC`-only configurations and add a comment so
this does not silently break.
--
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]
