You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "Weixin-Xu (via GitHub)" <gi...@apache.org> on 2026/10/19 10:54:30 UTC
[PR] [BOLT] Fix native memory lifecycle and shared UDF runtime [gluten]
Weixin-Xu opened a new pull request, #13367:
URL: https://github.com/apache/gluten/pull/13367
## What changes are proposed in this pull request?
This PR fixes native runtime issues encountered while running Gluten tests with the Bolt backend.
- Fix memory holder cleanup for driver contexts using task ID `-1`. Retain task memory accounting until all holder destructions finish, including concurrent destruction and registration of new holders.
- Pass the configured minimum-memory wait timeout to the execution memory pool.
- Finalize the S3 filesystem after stopping the backend IO executor.
- Derive the default partial-aggregation spill threshold from the abandonment thresholds, while preserving validation of explicitly configured values.
- Link example UDF/UDAF libraries and the UDF test against the shared backend engine to avoid duplicate engine registries. Export the required memory flags and relink the backend when its symbol map changes.
Add memory lifecycle regression tests covering driver cleanup, preservation of other managers, registration during destruction, and concurrent destruction.
Changes are confined to Bolt native implementation, build configuration, and tests.
## How was this patch tested?
These changes were tested as part of the integrated Bolt CI candidate using Bolt revision `071e54764f887dfb4337260e440f2f12f6ab82f5`:
- Native CTest: 143 tests passed, with 7 existing skips.
- JVM UDF tests: 61 tests passed, plus 2 GenerateExample tests.
- The complete reference TPC-H/TPC-DS/HDFS/S3 integration case passed, including process shutdown.
The integrated candidate was subsequently split into topic commits without changing its final file tree. Patch composition and dependency checks passed.
The standalone branch has not been rebuilt or runtime-tested independently. Native reproduction requires the recorded Bolt revision and applicable engine compatibility patches.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: Codex (GPT-6)
--
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] [BOLT] Fix native memory lifecycle and shared UDF runtime [gluten]
Posted by "liuneng1994 (via GitHub)" <gi...@apache.org>.
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: 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