github-actions[bot] commented on code in PR #67845:
URL: https://github.com/apache/doris/pull/67845#discussion_r4227851799
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -678,32 +715,64 @@ 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());
+ std::sort(exceeded_wgs.begin(), exceeded_wgs.end(),
+ [](const auto& lhs, const auto& rhs) { return lhs.first >
rhs.first; });
+
+ int64_t freed_mem = 0;
+ size_t tried_wgs = 0;
+ for (const auto& [snapshot_exceeded_memory, wg] : exceeded_wgs) {
+ if (snapshot_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(snapshot_exceeded_memory,
TUnit::BYTES)
+ << " less than 128MB, no need to revoke memory";
+ break;
+ }
+ // The snapshot above was taken before the earlier workload groups
were scanned, during
+ // which this one may have released memory (its queries finished or
were cancelled).
+ // Decide from its current usage: the min memory stays reserved for a
workload group that
+ // no longer exceeds it, and the amount to revoke is what it exceeds
by now.
+ const int64_t exceeded_memory = wg->total_mem_used() -
wg->min_memory_limit();
+ if (exceeded_memory < 1 << 27) {
+ LOG(INFO) << "The workload group " << wg->memory_debug_string()
+ << " exceeded its min memory by "
+ << PrettyPrinter::print(snapshot_exceeded_memory,
TUnit::BYTES)
+ << " when the walk started, now by "
+ << PrettyPrinter::print(exceeded_memory, TUnit::BYTES)
+ << " less than 128MB, skip it";
+ continue;
+ }
+ 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
+ freed_mem = wg->revoke_memory(need_free_mem, "exceed_memory",
profile.get());
Review Comment:
[P2] Recheck process pressure before cancelling a later peer. The requestor
is checked only before this walk. If an unreclaimable first peer returns zero
while another query or cache releases enough memory for the requestor's
reservation, this new loop still calls revoke_memory() on the next over-minimum
peer and cancels its active query. That call checks group usage, not whether
process pressure still exists, and its positive result skips the requestor's
later recovery check. Stop peer reclamation when the recorded reservation fits,
then resume the requestor; cover recovery during an unsuccessful first-peer
scan.
##########
be/test/runtime/workload_group/workload_group_manager_test.cpp:
##########
@@ -153,9 +175,127 @@ class WorkloadGroupManagerTest : public testing::Test {
}
}
+ // Helpers for the PROCESS_MEMORY_EXCEEDED cases.
+
+ // A workload group whose min memory limit is 100 MiB, so that a query
consuming more than
+ // that routes into handle_single_query_ directly, and a smaller one is
routed through the
+ // other workload groups first.
+ std::shared_ptr<WorkloadGroup> _create_wg_with_min_memory(uint64_t id) {
+ MemInfo::set_mem_limit_for_test(kProcessMemLimitForWg);
+ WorkloadGroupInfo wg_info {.id = id,
+ .memory_limit = kProcessMemLimitForWg,
+ .min_memory_percent = 10,
+ .max_memory_percent = 100};
+ auto wg = _wg_manager->get_or_create_workload_group(wg_info);
+ EXPECT_EQ(wg->min_memory_limit(), 1024L * 1024 * 100);
+ return wg;
+ }
+
+ // A query on `wg` that consumes `memory` bytes until TearDown.
+ std::shared_ptr<QueryContext>
_create_query_with_memory(std::shared_ptr<WorkloadGroup>& wg,
+ int64_t memory) {
+ auto query = _generate_on_query(wg);
+ query->query_mem_tracker()->consume(memory);
+ _consumed_memory.emplace_back(query, memory);
+ wg->refresh_memory_usage();
+ return query;
+ }
+
+ // Release `memory` bytes of `query` while a case has lowered the process
memory limit.
+ // refresh_memory_usage() also derives the workload group limits from the
process memory
+ // limit, so it runs under the limit the workload group was created with.
+ void _release_query_memory(const std::shared_ptr<WorkloadGroup>& wg,
+ const std::shared_ptr<QueryContext>& query,
int64_t memory) {
+ query->query_mem_tracker()->consume(-memory);
+ _consumed_memory.emplace_back(query, -memory);
+ const int64_t mem_limit = MemInfo::mem_limit();
+ MemInfo::set_mem_limit_for_test(kProcessMemLimitForWg);
+ wg->refresh_memory_usage();
+ MemInfo::set_mem_limit_for_test(mem_limit);
+ ASSERT_EQ(wg->min_memory_limit(), 1024L * 1024 * 100);
+ }
+
+ // Process soft memory limit is exceeded, hard memory limit is not.
+ static void _exceed_process_soft_mem_limit() {
+ MemInfo::set_mem_limit_for_test(kLargeProcessMemLimit);
+ MemInfo::set_soft_mem_limit_for_test(1);
+
ASSERT_TRUE(GlobalMemoryArbitrator::is_exceed_soft_mem_limit(kProcessPausedReserveSize));
+ ASSERT_FALSE(GlobalMemoryArbitrator::is_exceed_hard_mem_limit());
Review Comment:
[P2] Control both sides of the memory predicates in these tests. This helper
raises the process hard limit, but is_exceed_hard_mem_limit() also checks
system available memory; a runner below the low watermark fails this assertion
before the manager runs. The recovery cases likewise leave the
warning-watermark check live, while the dedicated system-boundary case assumes
the runner's original process soft limit has headroom. Save and restore a
controlled available-memory value above the warning mark for process-limit
cases, and set explicit process headroom in the system-boundary case.
--
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]
