From 66524c8121fd7534d8a2ec5d88ab25d7c3dc4ffa Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Fri, 1 Aug 2025 12:23:58 +0200 Subject: [PATCH 1/6] 8364531 Hi all, please review this refactoring, eliminating some copy&paste, in cset candidate group liveness logging. The output is mostly the same, except for the young cset group, where the "-" has been replaced to just print the default value (0.0) for efficiency. I did not see this important to have, the gc efficiency for young gen isn't interesting. Testing: local compilation, gha, visual inspection of output Thanks, Thomas --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 63 +++++++------------- src/hotspot/share/gc/g1/g1ConcurrentMark.hpp | 4 +- 2 files changed, 25 insertions(+), 42 deletions(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index c728c11d5cd52..3e80d4d7b5e64 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -2975,7 +2975,6 @@ G1CMTask::G1CMTask(uint worker_id, #define G1PPRL_BYTE_FORMAT " %9zu" #define G1PPRL_BYTE_H_FORMAT " %9s" #define G1PPRL_DOUBLE_FORMAT "%14.1f" -#define G1PPRL_GCEFF_FORMAT " %14s" #define G1PPRL_GCEFF_H_FORMAT " %14s" #define G1PPRL_GID_H_FORMAT " %9s" #define G1PPRL_GID_FORMAT " " UINT32_FORMAT_W(9) @@ -3090,7 +3089,7 @@ G1PrintRegionLivenessInfoClosure::~G1PrintRegionLivenessInfoClosure() { // add static memory usages to remembered set sizes _total_remset_bytes += G1HeapRegionRemSet::static_mem_size(); - do_cset_groups(); + log_cset_candidate_groups(); // Print the footer of the output. log_trace(gc, liveness)(G1PPRL_LINE_PREFIX); @@ -3110,10 +3109,24 @@ G1PrintRegionLivenessInfoClosure::~G1PrintRegionLivenessInfoClosure() { bytes_to_mb(_total_code_roots_bytes)); } -void G1PrintRegionLivenessInfoClosure::do_cset_groups() { +void G1PrintRegionLivenessInfoClosure::log_cset_candidate_group(G1CSetCandidateGroup* group, const char* type) { + log_trace(gc, liveness)(G1PPRL_LINE_PREFIX + G1PPRL_GID_FORMAT + G1PPRL_LEN_FORMAT + G1PPRL_GID_GCEFF_FORMAT + G1PPRL_BYTE_FORMAT + G1PPRL_BYTE_FORMAT + G1PPRL_TYPE_H_FORMAT, + group->group_id(), group->length(), + group->gc_efficiency(), + group->liveness(), group->card_set()->mem_size(), + type); +} + +void G1PrintRegionLivenessInfoClosure::log_cset_candidate_groups() { log_trace(gc, liveness)(G1PPRL_LINE_PREFIX); - log_trace(gc, liveness)(G1PPRL_LINE_PREFIX" Collectionset Candidate Groups"); - log_trace(gc, liveness)(G1PPRL_LINE_PREFIX " Types: Y=Young Regions, M=From Marking Regions, R=Retained Regions"); + log_trace(gc, liveness)(G1PPRL_LINE_PREFIX" Collection Set Candidate Groups"); + log_trace(gc, liveness)(G1PPRL_LINE_PREFIX " Types: Y=Young, M=From Marking Regions, R=Retained Regions"); log_trace(gc, liveness)(G1PPRL_LINE_PREFIX G1PPRL_GID_H_FORMAT G1PPRL_LEN_H_FORMAT @@ -3137,49 +3150,17 @@ void G1PrintRegionLivenessInfoClosure::do_cset_groups() { "(bytes)", ""); G1CollectedHeap* g1h = G1CollectedHeap::heap(); - G1CSetCandidateGroup* young_only_cset_group =g1h->young_regions_cset_group(); - _total_remset_bytes += young_only_cset_group->card_set()->mem_size(); - - log_trace(gc, liveness)(G1PPRL_LINE_PREFIX - G1PPRL_GID_FORMAT - G1PPRL_LEN_FORMAT - G1PPRL_GCEFF_FORMAT - G1PPRL_BYTE_FORMAT - G1PPRL_BYTE_FORMAT - G1PPRL_TYPE_H_FORMAT, - young_only_cset_group->group_id(), young_only_cset_group->length(), - "-", - size_t(0), young_only_cset_group->card_set()->mem_size(), - "Y"); + log_cset_candidate_group(g1h->young_regions_cset_group(), "Y"); + _total_remset_bytes += g1h->young_regions_cset_group()->card_set()->mem_size(); for (G1CSetCandidateGroup* group : g1h->policy()->candidates()->from_marking_groups()) { + log_cset_candidate_group(group, "M"); _total_remset_bytes += group->card_set()->mem_size(); - log_trace(gc, liveness)(G1PPRL_LINE_PREFIX - G1PPRL_GID_FORMAT - G1PPRL_LEN_FORMAT - G1PPRL_GID_GCEFF_FORMAT - G1PPRL_BYTE_FORMAT - G1PPRL_BYTE_FORMAT - G1PPRL_TYPE_H_FORMAT, - group->group_id(), group->length(), - group->gc_efficiency(), - group->liveness(), group->card_set()->mem_size(), - "M"); } for (G1CSetCandidateGroup* group : g1h->policy()->candidates()->retained_groups()) { + log_cset_candidate_group(group, "R"); _total_remset_bytes += group->card_set()->mem_size(); - log_trace(gc, liveness)(G1PPRL_LINE_PREFIX - G1PPRL_GID_FORMAT - G1PPRL_LEN_FORMAT - G1PPRL_GID_GCEFF_FORMAT - G1PPRL_BYTE_FORMAT - G1PPRL_BYTE_FORMAT - G1PPRL_TYPE_H_FORMAT, - group->group_id(), group->length(), - group->gc_efficiency(), - group->liveness(), group->card_set()->mem_size(), - "R"); } } diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp index 3c3416ebcadbf..86c961893da4d 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp @@ -42,6 +42,7 @@ class ConcurrentGCTimer; class G1CollectedHeap; +class G1CSetCandidateGroup; class G1ConcurrentMark; class G1ConcurrentMarkThread; class G1CMOopClosure; @@ -974,7 +975,8 @@ class G1PrintRegionLivenessInfoClosure : public G1HeapRegionClosure { return (double) val / (double) M; } - void do_cset_groups(); + void log_cset_candidate_group(G1CSetCandidateGroup* gr, const char* type); + void log_cset_candidate_groups(); public: // The header and footer are printed in the constructor and From d1653627e0b3c86b2bf7687457b3f179970a4a9a Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Fri, 1 Aug 2025 14:28:17 +0200 Subject: [PATCH 2/6] * use one line per parameter in log message --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index 3e80d4d7b5e64..7c8a58c2ca513 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -3117,9 +3117,11 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_group(G1CSetCandidateG G1PPRL_BYTE_FORMAT G1PPRL_BYTE_FORMAT G1PPRL_TYPE_H_FORMAT, - group->group_id(), group->length(), + group->group_id(), + group->length(), group->gc_efficiency(), - group->liveness(), group->card_set()->mem_size(), + group->liveness(), + group->card_set()->mem_size(), type); } From 6f605b70176694e9148337b8857fbbc5bdf8a50f Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Fri, 1 Aug 2025 15:29:45 +0200 Subject: [PATCH 3/6] * more refactoring --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 19 ++++++++++--------- src/hotspot/share/gc/g1/g1ConcurrentMark.hpp | 2 ++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index 7c8a58c2ca513..7f049d84f3852 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -3125,6 +3125,13 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_group(G1CSetCandidateG type); } +void G1PrintRegionLivenessInfoClosure::log_cset_candidate_grouplist(G1CSetCandidateGroupList& gl, const char* type) { + for (G1CSetCandidateGroup* group : gl) { + log_cset_candidate_group(group, type); + _total_remset_bytes += group->card_set()->mem_size(); + } +} + void G1PrintRegionLivenessInfoClosure::log_cset_candidate_groups() { log_trace(gc, liveness)(G1PPRL_LINE_PREFIX); log_trace(gc, liveness)(G1PPRL_LINE_PREFIX" Collection Set Candidate Groups"); @@ -3156,13 +3163,7 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_groups() { log_cset_candidate_group(g1h->young_regions_cset_group(), "Y"); _total_remset_bytes += g1h->young_regions_cset_group()->card_set()->mem_size(); - for (G1CSetCandidateGroup* group : g1h->policy()->candidates()->from_marking_groups()) { - log_cset_candidate_group(group, "M"); - _total_remset_bytes += group->card_set()->mem_size(); - } - - for (G1CSetCandidateGroup* group : g1h->policy()->candidates()->retained_groups()) { - log_cset_candidate_group(group, "R"); - _total_remset_bytes += group->card_set()->mem_size(); - } + G1CollectionSetCandidates * candidates = g1h->policy()->candidates(); + log_cset_candidate_grouplist(candidates->from_marking_groups(), "M"); + log_cset_candidate_grouplist(candidates->retained_groups(), "R"); } diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp index 86c961893da4d..95c3d056d48ab 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp @@ -43,6 +43,7 @@ class ConcurrentGCTimer; class G1CollectedHeap; class G1CSetCandidateGroup; +class G1CSetCandidateGroupList; class G1ConcurrentMark; class G1ConcurrentMarkThread; class G1CMOopClosure; @@ -976,6 +977,7 @@ class G1PrintRegionLivenessInfoClosure : public G1HeapRegionClosure { } void log_cset_candidate_group(G1CSetCandidateGroup* gr, const char* type); + void log_cset_candidate_grouplist(G1CSetCandidateGroupList& gl, const char* type); void log_cset_candidate_groups(); public: From cb77371dbd41fb2e72f647d1ecbce3c6e01171e0 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Mon, 4 Aug 2025 11:40:12 +0200 Subject: [PATCH 4/6] * fix typo --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index 7f049d84f3852..69ceab20a09b2 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -3163,7 +3163,7 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_groups() { log_cset_candidate_group(g1h->young_regions_cset_group(), "Y"); _total_remset_bytes += g1h->young_regions_cset_group()->card_set()->mem_size(); - G1CollectionSetCandidates * candidates = g1h->policy()->candidates(); + G1CollectionSetCandidates* candidates = g1h->policy()->candidates(); log_cset_candidate_grouplist(candidates->from_marking_groups(), "M"); log_cset_candidate_grouplist(candidates->retained_groups(), "R"); } From 6c51c94175312342fc556e29c2aa1a36a2846e4c Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Tue, 5 Aug 2025 09:55:30 +0200 Subject: [PATCH 5/6] * sangheon review --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 8 ++++---- src/hotspot/share/gc/g1/g1ConcurrentMark.hpp | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index 69ceab20a09b2..cb89c057914d4 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -3109,7 +3109,7 @@ G1PrintRegionLivenessInfoClosure::~G1PrintRegionLivenessInfoClosure() { bytes_to_mb(_total_code_roots_bytes)); } -void G1PrintRegionLivenessInfoClosure::log_cset_candidate_group(G1CSetCandidateGroup* group, const char* type) { +void G1PrintRegionLivenessInfoClosure::log_cset_candidate_group_add_total(G1CSetCandidateGroup* group, const char* type) { log_trace(gc, liveness)(G1PPRL_LINE_PREFIX G1PPRL_GID_FORMAT G1PPRL_LEN_FORMAT @@ -3123,12 +3123,12 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_group(G1CSetCandidateG group->liveness(), group->card_set()->mem_size(), type); + _total_remset_bytes += group->card_set()->mem_size(); } void G1PrintRegionLivenessInfoClosure::log_cset_candidate_grouplist(G1CSetCandidateGroupList& gl, const char* type) { for (G1CSetCandidateGroup* group : gl) { - log_cset_candidate_group(group, type); - _total_remset_bytes += group->card_set()->mem_size(); + log_cset_candidate_group_add_total(group, type); } } @@ -3160,7 +3160,7 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_groups() { G1CollectedHeap* g1h = G1CollectedHeap::heap(); - log_cset_candidate_group(g1h->young_regions_cset_group(), "Y"); + log_cset_candidate_group_add_total(g1h->young_regions_cset_group(), "Y"); _total_remset_bytes += g1h->young_regions_cset_group()->card_set()->mem_size(); G1CollectionSetCandidates* candidates = g1h->policy()->candidates(); diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp index 95c3d056d48ab..4977da4729de2 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.hpp @@ -976,7 +976,7 @@ class G1PrintRegionLivenessInfoClosure : public G1HeapRegionClosure { return (double) val / (double) M; } - void log_cset_candidate_group(G1CSetCandidateGroup* gr, const char* type); + void log_cset_candidate_group_add_total(G1CSetCandidateGroup* gr, const char* type); void log_cset_candidate_grouplist(G1CSetCandidateGroupList& gl, const char* type); void log_cset_candidate_groups(); From cc413ce77e54569ae814535d0aacfb9e802b1e1b Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Tue, 5 Aug 2025 11:06:38 +0200 Subject: [PATCH 6/6] * ayang review --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index cb89c057914d4..c5b8c89e2f047 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -3161,7 +3161,6 @@ void G1PrintRegionLivenessInfoClosure::log_cset_candidate_groups() { G1CollectedHeap* g1h = G1CollectedHeap::heap(); log_cset_candidate_group_add_total(g1h->young_regions_cset_group(), "Y"); - _total_remset_bytes += g1h->young_regions_cset_group()->card_set()->mem_size(); G1CollectionSetCandidates* candidates = g1h->policy()->candidates(); log_cset_candidate_grouplist(candidates->from_marking_groups(), "M");