From b3dd0084518ae0320a2a9741d9836ec62e1fcca9 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Mon, 3 Mar 2025 19:18:24 +0100 Subject: [PATCH] ayang review 2 * removal of useless code * renamings --- .../x86/gc/g1/g1BarrierSetAssembler_x86.cpp | 10 +++--- .../share/gc/g1/g1CardTableClaimTable.cpp | 10 +++--- .../share/gc/g1/g1CardTableClaimTable.hpp | 4 +-- .../share/gc/g1/g1ConcurrentRefine.cpp | 2 +- src/hotspot/share/gc/g1/g1RemSet.cpp | 12 +++---- src/hotspot/share/gc/g1/g1ThreadLocalData.hpp | 6 ++++ .../gc/g1/g1YoungGCPostEvacuateTasks.cpp | 32 +++++++++---------- 7 files changed, 41 insertions(+), 35 deletions(-) diff --git a/src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp b/src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp index 6405301ba6a88..c952f0d21bc20 100644 --- a/src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp +++ b/src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp @@ -103,7 +103,7 @@ void G1BarrierSetAssembler::gen_write_ref_array_post_barrier(MacroAssembler* mas Label done; __ testptr(count, count); - __ jcc(Assembler::equal, done); + __ jcc(Assembler::zero, done); // Calculate end address in "count". Address::ScaleFactor scale = UseCompressedOops ? Address::times_4 : Address::times_8; @@ -129,8 +129,10 @@ void G1BarrierSetAssembler::gen_write_ref_array_post_barrier(MacroAssembler* mas __ bind(loop); Label is_clean_card; - __ cmpb(Address(addr, 0), G1CardTable::clean_card_val()); - __ jcc(Assembler::equal, is_clean_card); + if (UseCondCardMark) { + __ cmpb(Address(addr, 0), G1CardTable::clean_card_val()); + __ jcc(Assembler::equal, is_clean_card); + } Label next_card; __ bind(next_card); @@ -320,7 +322,7 @@ static void generate_post_barrier_fast_path(MacroAssembler* masm, #ifdef _LP64 assert(thread == r15_thread, "must be"); #endif // _LP64 - assert_different_registers(store_addr, new_val, thread, tmp1 /*, tmp2 unused */, noreg); + assert_different_registers(store_addr, new_val, thread, tmp1 /*, tmp2 unused for x86 */, noreg); // Does store cross heap regions? __ movptr(tmp1, store_addr); // tmp1 := store address diff --git a/src/hotspot/share/gc/g1/g1CardTableClaimTable.cpp b/src/hotspot/share/gc/g1/g1CardTableClaimTable.cpp index 781f3b955c3e3..e0cadbdd907da 100644 --- a/src/hotspot/share/gc/g1/g1CardTableClaimTable.cpp +++ b/src/hotspot/share/gc/g1/g1CardTableClaimTable.cpp @@ -46,17 +46,17 @@ void G1CardTableClaimTable::initialize(uint max_reserved_regions) { assert(_card_claims == nullptr, "Must not be initialized twice"); _card_claims = NEW_C_HEAP_ARRAY(uint, max_reserved_regions, mtGC); _max_reserved_regions = max_reserved_regions; - reset_all_claims_to_unclaimed(); + reset_all_to_unclaimed(); } -void G1CardTableClaimTable::reset_all_claims_to_unclaimed() { - for (size_t i = 0; i < _max_reserved_regions; i++) { +void G1CardTableClaimTable::reset_all_to_unclaimed() { + for (uint i = 0; i < _max_reserved_regions; i++) { _card_claims[i] = 0; } } -void G1CardTableClaimTable::reset_all_claims_to_claimed() { - for (size_t i = 0; i < _max_reserved_regions; i++) { +void G1CardTableClaimTable::reset_all_to_claimed() { + for (uint i = 0; i < _max_reserved_regions; i++) { _card_claims[i] = (uint)G1HeapRegion::CardsPerRegion; } } diff --git a/src/hotspot/share/gc/g1/g1CardTableClaimTable.hpp b/src/hotspot/share/gc/g1/g1CardTableClaimTable.hpp index 4529646e2e7e5..4f524b83f976d 100644 --- a/src/hotspot/share/gc/g1/g1CardTableClaimTable.hpp +++ b/src/hotspot/share/gc/g1/g1CardTableClaimTable.hpp @@ -59,8 +59,8 @@ class G1CardTableClaimTable : public CHeapObj { // Allocates the data structure and initializes the claims to unclaimed. void initialize(uint max_reserved_regions); - void reset_all_claims_to_unclaimed(); - void reset_all_claims_to_claimed(); + void reset_all_to_unclaimed(); + void reset_all_to_claimed(); inline bool has_unclaimed_cards(uint region); inline void reset_to_unclaimed(uint region); diff --git a/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp b/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp index 792758986d782..294fce9c2d5f7 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp @@ -346,7 +346,7 @@ bool G1ConcurrentRefineWorkState::complete(bool concurrent, bool print_log) { void G1ConcurrentRefineWorkState::snapshot_heap_into(G1CardTableClaimTable* sweep_table) { // G1CollectedHeap::heap_region_iterate() below will only visit committed regions. Initialize // all entries in the state table here to not require special handling when iterating over it. - sweep_table->reset_all_claims_to_claimed(); + sweep_table->reset_all_to_claimed(); class SnapshotRegionsClosure : public G1HeapRegionClosure { G1CardTableClaimTable* _sweep_table; diff --git a/src/hotspot/share/gc/g1/g1RemSet.cpp b/src/hotspot/share/gc/g1/g1RemSet.cpp index 0577bd2f9fd78..2584d01580b9b 100644 --- a/src/hotspot/share/gc/g1/g1RemSet.cpp +++ b/src/hotspot/share/gc/g1/g1RemSet.cpp @@ -268,7 +268,7 @@ class G1ClearCardTableTask : public G1AbstractSubTask { // regions. //assert(_next_dirty_regions->size() == 0, "next dirty regions must be empty"); - _card_claim_table.reset_all_claims_to_unclaimed(); + _card_claim_table.reset_all_to_unclaimed(); } void complete_evac_phase(bool merge_dirty_regions) { @@ -829,12 +829,12 @@ class MergeRefinementTableTask : public WorkerTask { size_t* card_cur_word = (size_t*)card_table->byte_for_index(start_idx); - size_t* refinement_cur_card = (size_t*)refinement_table->byte_for_index(start_idx); - size_t* const refinement_end_card = refinement_cur_card + claim.size() / (sizeof(size_t) / sizeof(G1CardTable::CardValue)); + size_t* refinement_cur_word = (size_t*)refinement_table->byte_for_index(start_idx); + size_t* const refinement_end_word = refinement_cur_word + claim.size() / (sizeof(size_t) / sizeof(G1CardTable::CardValue)); - for (; refinement_cur_card < refinement_end_card; ++refinement_cur_card, ++card_cur_word) { - size_t value = *refinement_cur_card; - *refinement_cur_card = G1CardTable::WordAllClean; + for (; refinement_cur_word < refinement_end_word; ++refinement_cur_word, ++card_cur_word) { + size_t value = *refinement_cur_word; + *refinement_cur_word = G1CardTable::WordAllClean; // Dirty is "0", so we need to logically-and here. This is also safe // for all other possible values in the card table; at this point this // can be either g1_dirty_card or g1_to_cset_card which will both be diff --git a/src/hotspot/share/gc/g1/g1ThreadLocalData.hpp b/src/hotspot/share/gc/g1/g1ThreadLocalData.hpp index cf9d45316e93b..67886149587f2 100644 --- a/src/hotspot/share/gc/g1/g1ThreadLocalData.hpp +++ b/src/hotspot/share/gc/g1/g1ThreadLocalData.hpp @@ -91,6 +91,12 @@ class G1ThreadLocalData { data(thread)->_byte_map_base = new_byte_map_base; } +#ifdef ASSERT + static G1CardTable::CardValue* get_byte_map_base(Thread* thread) { + return data(thread)->_byte_map_base; + } +#endif + static G1RegionPinCache& pin_count_cache(Thread* thread) { return data(thread)->_pin_cache; } diff --git a/src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp b/src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp index 11abe446374b6..a299f82cca776 100644 --- a/src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp +++ b/src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp @@ -830,43 +830,41 @@ class G1PostEvacuateCollectionSetCleanupTask2::FreeCollectionSetTask : public G1 class G1PostEvacuateCollectionSetCleanupTask2::ResizeTLABsAndSwapCardTableTask : public G1AbstractSubTask { G1JavaThreadsListClaimer _claimer; - volatile bool _non_java_threads_claim; // There is not much work per thread so the number of threads per worker is high. static const uint ThreadsPerWorker = 250; public: ResizeTLABsAndSwapCardTableTask() - : G1AbstractSubTask(G1GCPhaseTimes::ResizeThreadLABs), _claimer(ThreadsPerWorker), _non_java_threads_claim(false) + : G1AbstractSubTask(G1GCPhaseTimes::ResizeThreadLABs), _claimer(ThreadsPerWorker) { G1BarrierSet::g1_barrier_set()->swap_global_card_table(); - } - void do_work(uint worker_id) override { - class SwapCardTableClosure : public ThreadClosure { +#ifdef ASSERT + class AssertCardTableBaseNull : public ThreadClosure { public: + void do_thread(Thread* thread) { - // The global card table references have already been swapped. - G1CardTable::CardValue* new_card_table_base = G1CollectedHeap::heap()->card_table_base(); - G1ThreadLocalData::set_byte_map_base(thread, new_card_table_base); + assert(G1ThreadLocalData::get_byte_map_base(thread) == nullptr, "thread " PTR_FORMAT " (%s) has non-null card table base", p2i(thread), thread->name()); } - } swap_cl; + } assert_cl; + Threads::non_java_threads_do(&assert_cl); +#endif + } - // We do not expect too many non-Java threads compared to Java threads, so just - // let one worker claim that work. - if (!_non_java_threads_claim && !Atomic::cmpxchg(&_non_java_threads_claim, false, true, memory_order_relaxed)) { - Threads::non_java_threads_do(&swap_cl); - } + void do_work(uint worker_id) override { class ResizeAndSwapCardTableClosure : public ThreadClosure { - SwapCardTableClosure _cl; - public: + void do_thread(Thread* thread) { if (UseTLAB && ResizeTLAB) { static_cast(thread)->tlab().resize(); } - _cl.do_thread(thread); + + // The global card table references have already been swapped. + G1CardTable::CardValue* new_card_table_base = G1CollectedHeap::heap()->card_table_base(); + G1ThreadLocalData::set_byte_map_base(thread, new_card_table_base); } } resize_and_swap_cl;