mrhhsg commented on code in PR #67845:
URL: https://github.com/apache/doris/pull/67845#discussion_r4227296449
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -678,33 +702,38 @@ int64_t
WorkloadGroupMgr::revoke_memory_from_other_groups_() {
// then not revoke memory from it.
continue;
}
- if (total_used_memory - min_memory_limit > max_exceeded_memory) {
- max_wg = workload_group.second;
- max_exceeded_memory = total_used_memory - min_memory_limit;
- }
+ exceeded_wgs.emplace_back(total_used_memory - min_memory_limit,
workload_group.second);
}
}
- if (max_wg == nullptr) {
- return 0;
- }
- if (max_exceeded_memory < 1 << 27) {
- LOG(INFO) << "The workload group that exceed most memory is :"
- << max_wg->memory_debug_string() << ", max_exceeded_memory: "
- << PrettyPrinter::print(max_exceeded_memory, TUnit::BYTES)
- << " less than 128MB, no need to revoke memory";
- return 0;
- }
- int64_t freed_mem = static_cast<int64_t>((double)max_exceeded_memory *
0.1);
- // Revoke 10% of memory from the workload group that exceed most memory
- max_wg->revoke_memory(freed_mem, "exceed_memory", profile.get());
- std::stringstream ss;
- profile->pretty_print(&ss);
- LOG(INFO) << fmt::format(
- "[MemoryGC] process memory not enough, revoke memory from
workload_group: {}, "
- "free memory {}. cost(us): {}, details: {}",
- max_wg->memory_debug_string(),
PrettyPrinter::print_bytes(freed_mem),
- watch.elapsed_time() / 1000, ss.str());
- return freed_mem;
+ std::sort(exceeded_wgs.begin(), exceeded_wgs.end(),
+ [](const auto& lhs, const auto& rhs) { return lhs.first >
rhs.first; });
+
+ for (const auto& [exceeded_memory, wg] : exceeded_wgs) {
+ if (exceeded_memory < 1 << 27) {
+ // The remaining workload groups exceed even less.
+ LOG(INFO) << "The workload group that exceed most memory among the
untried ones is :"
+ << wg->memory_debug_string() << ", exceeded_memory: "
+ << PrettyPrinter::print(exceeded_memory, TUnit::BYTES)
+ << " less than 128MB, no need to revoke memory";
+ break;
+ }
+ auto need_free_mem = static_cast<int64_t>((double)exceeded_memory *
0.1);
+ // Revoke 10% of memory from the workload group that exceed most memory
+ int64_t freed_mem = wg->revoke_memory(need_free_mem, "exceed_memory",
profile.get());
+ std::stringstream ss;
+ profile->pretty_print(&ss);
Review Comment:
Fixed in a7895decb8b. `revoke_memory_from_other_groups_()` no longer
pretty-prints the accumulated profile after every `wg->revoke_memory()` call.
Each tried group now gets a one-line log (need/freed/cost) and the profile is
formatted once after the walk, so a walk over N groups emits N children once
instead of 1+2+...+N. The per-group details are still logged by
`WorkloadGroup::revoke_memory()` itself. The return value is unchanged;
`WorkloadGroupManagerTest.*` (45 cases, including
`process_mem_exceeded_below_min_memory_tries_next_peer`) pass.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -824,38 +839,45 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
return true;
}
} else {
- // Should not consider about process memory. For example, the query's
limit is 100g, workload
- // group's memlimit is 10g, process memory is 20g. The query reserve
will always failed in wg
- // limit, and process is always have memory, so that it will resume
and failed reserve again.
- const size_t test_memory_size = std::max<size_t>(size_to_reserve, 32L
* 1024 * 1024);
- if
(!GlobalMemoryArbitrator::is_exceed_soft_mem_limit(test_memory_size)) {
- LOG(INFO) << "Query: " << query_id
- << ", process limit not exceeded now, resume this query"
- << ", process memory info: "
- <<
GlobalMemoryArbitrator::process_memory_used_details_str()
- << ", wg info: " << wg->debug_string();
- requestor->task_controller()->set_memory_sufficient(true);
- return true;
- } else {
- // if cannot find any memory to release, then let the query
continue to run as far as possible
- // or cancelled by gc if memory is really not enough.
- Status error_status = Status::MemoryLimitExceeded(
- "Query {} process memory is exceeded"
- ", and there is no cache now. And could not find task to
spill, disable "
- "reserve memory and resume it. "
- "Query memory usage: {}, limit: {}, reserved "
- "size: {}, try to reserve: {}, wg info: {}."
- " Maybe you should set the workload group's limit to a
lower value. {}",
- query_id, PrettyPrinter::print_bytes(memory_usage),
- PrettyPrinter::print_bytes(limit),
PrettyPrinter::print_bytes(reserved_size),
- PrettyPrinter::print_bytes(size_to_reserve),
wg->memory_debug_string(),
- doris::ProcessProfile::instance()
- ->memory_profile()
- ->process_memory_detail_str());
- LOG_LONG_STRING(INFO, error_status.to_string());
- requestor->task_controller()->cancel(error_status);
- return true;
+ // PROCESS_MEMORY_EXCEEDED. The caller
(handle_process_memory_exceeded_) has already
+ // resumed the query if the process is no longer above the soft memory
limit, so the
Review Comment:
On the two parts carried forward in the latest review (the spill callback
resuming without freeing memory, and a zero reservation skipping the reserve
check): both are pre-existing master behavior outside this manager, and the
hard-limit protection still holds without memory gc.
1. `QueryTaskController::revoke_memory()` resumes the query when its own
snapshot finds no revocable task. The query then retries the reservation; while
the process is still over the limit the reserve fails again and the query comes
back to `resolve_process_memory_exceeded_query_()` without revocable tasks,
where it is cancelled at once at the hard limit.
`process_mem_exceeded_spills_revocable_tasks_at_hard_limit` covers exactly this
re-pause after a spill that freed nothing.
2. The `reserve_size > 0` gate in `PipelineTask` is unchanged by this PR. An
operator that reserves nothing still goes through
`Allocator::sys_memory_check()` for every allocation, which checks
`GlobalMemoryArbitrator::is_exceed_hard_mem_limit(size)` directly and, with
`disable_memory_gc=true`, does not wait and throws `MEM_ALLOC_FAILED`
immediately, so the query is cancelled by the allocator rather than running
past the hard limit.
So no change in this PR; the paused-query manager converges in both cases
and the allocator enforces the hard limit independently of `WorkloadGroupMgr`.
--
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]
