From e5ce3984284b0c21bc9f38e8a970309eac089cac Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Fri, 21 Mar 2025 15:51:24 +0100 Subject: [PATCH 1/2] 8271870 split evac fail reigons. need cleanup refactoring * undo some changes in test --- .../share/gc/g1/g1ParScanThreadState.cpp | 111 ++++++++++-------- .../share/gc/g1/g1ParScanThreadState.hpp | 10 +- .../jtreg/gc/g1/TestAllocationFailure.java | 3 +- 3 files changed, 69 insertions(+), 55 deletions(-) diff --git a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp index 298222b35b5b8..04fd343a3ef49 100644 --- a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp +++ b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp @@ -238,8 +238,7 @@ void G1ParScanThreadState::do_partial_array(PartialArrayState* state, bool stole } MAYBE_INLINE_EVACUATION -void G1ParScanThreadState::start_partial_objarray(G1HeapRegionAttr dest_attr, - oop from_obj, +void G1ParScanThreadState::start_partial_objarray(oop from_obj, oop to_obj) { assert(from_obj->is_forwarded(), "precondition"); assert(from_obj->forwardee() == to_obj, "precondition"); @@ -251,12 +250,6 @@ void G1ParScanThreadState::start_partial_objarray(G1HeapRegionAttr dest_attr, // The source array is unused when processing states. _partial_array_splitter.start(_task_queue, nullptr, to_array, array_length); - // Skip the card enqueue iff the object (to_array) is in survivor region. - // However, G1HeapRegion::is_survivor() is too expensive here. - // Instead, we use dest_attr.is_young() because the two values are always - // equal: successfully allocated young regions must be survivor regions. - assert(dest_attr.is_young() == _g1h->heap_region_containing(to_array)->is_survivor(), "must be"); - G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_young()); // Process the initial chunk. No need to process the type in the // klass, as it will already be handled by processing the built-in // module. @@ -422,6 +415,44 @@ void G1ParScanThreadState::update_bot_after_copying(oop obj, size_t word_sz) { region->update_bot_for_block(obj_start, obj_start + word_sz); } +ALWAYSINLINE +void G1ParScanThreadState::do_iterate_object(oop const obj, + oop const old, + Klass* const klass, + G1HeapRegionAttr const region_attr, + G1HeapRegionAttr const dest_attr, + uint age) { + // Most objects are not arrays, so do one array check rather than + // checking for each array category for each object. + if (klass->is_array_klass()) { + assert(!klass->is_stack_chunk_instance_klass(), "must be"); + + if (klass->is_objArray_klass()) { + start_partial_objarray(old, obj); + } else { + // Nothing needs to be done for typeArrays. Body doesn't contain + // any oops to scan, and the type in the klass will already be handled + // by processing the built-in module. + assert(klass->is_typeArray_klass(), "invariant"); + } + return; + } + + ContinuationGCSupport::transform_stack_chunk(obj); + + // Check for deduplicating young Strings. + if (G1StringDedup::is_candidate_from_evacuation(klass, + region_attr, + dest_attr, + age)) { + // Record old; request adds a new weak reference, which reference + // processing expects to refer to a from-space object. + _string_dedup_requests.add(old); + } + + obj->oop_iterate_backwards(&_scanner, klass); +} + // Private inline function, for direct internal use and providing the // implementation of the public not-inline function. MAYBE_INLINE_EVACUATION @@ -446,7 +477,7 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio // JNI only allows pinning of typeArrays, so we only need to keep those in place. if (region_attr.is_pinned() && klass->is_typeArray_klass()) { - return handle_evacuation_failure_par(old, old_mark, word_sz, true /* cause_pinned */); + return handle_evacuation_failure_par(old, old_mark, klass, region_attr, word_sz, true /* cause_pinned */); } uint age = 0; @@ -463,7 +494,7 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio if (obj_ptr == nullptr) { // This will either forward-to-self, or detect that someone else has // installed a forwarding pointer. - return handle_evacuation_failure_par(old, old_mark, word_sz, false /* cause_pinned */); + return handle_evacuation_failure_par(old, old_mark, klass, region_attr, word_sz, false /* cause_pinned */); } } @@ -475,7 +506,7 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio // Doing this after all the allocation attempts also tests the // undo_allocation() method too. undo_allocation(dest_attr, obj_ptr, word_sz, node_index); - return handle_evacuation_failure_par(old, old_mark, word_sz, false /* cause_pinned */); + return handle_evacuation_failure_par(old, old_mark, klass, region_attr, word_sz, false /* cause_pinned */); } // We're going to allocate linearly, so might as well prefetch ahead. @@ -507,39 +538,17 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio update_bot_after_copying(obj, word_sz); } - // Most objects are not arrays, so do one array check rather than - // checking for each array category for each object. - if (klass->is_array_klass()) { - if (klass->is_objArray_klass()) { - start_partial_objarray(dest_attr, old, obj); - } else { - // Nothing needs to be done for typeArrays. Body doesn't contain - // any oops to scan, and the type in the klass will already be handled - // by processing the built-in module. - assert(klass->is_typeArray_klass(), "invariant"); - } - return obj; - } - - ContinuationGCSupport::transform_stack_chunk(obj); - - // Check for deduplicating young Strings. - if (G1StringDedup::is_candidate_from_evacuation(klass, - region_attr, - dest_attr, - age)) { - // Record old; request adds a new weak reference, which reference - // processing expects to refer to a from-space object. - _string_dedup_requests.add(old); + { + // Skip the card enqueue iff the object (obj) is in survivor region. + // However, G1HeapRegion::is_survivor() is too expensive here. + // Instead, we use dest_attr.is_young() because the two values are always + // equal: successfully allocated young regions must be survivor regions. + assert(dest_attr.is_young() == _g1h->heap_region_containing(obj)->is_survivor(), "must be"); + G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_young()); + + do_iterate_object(obj, old, klass, region_attr, dest_attr, age); } - // Skip the card enqueue iff the object (obj) is in survivor region. - // However, G1HeapRegion::is_survivor() is too expensive here. - // Instead, we use dest_attr.is_young() because the two values are always - // equal: successfully allocated young regions must be survivor regions. - assert(dest_attr.is_young() == _g1h->heap_region_containing(obj)->is_survivor(), "must be"); - G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_young()); - obj->oop_iterate_backwards(&_scanner, klass); return obj; } else { _plab_allocator->undo_allocation(dest_attr, obj_ptr, word_sz, node_index); @@ -621,7 +630,7 @@ void G1ParScanThreadState::record_evacuation_failed_region(G1HeapRegion* r, uint } NOINLINE -oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, size_t word_sz, bool cause_pinned) { +oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, Klass* klass, G1HeapRegionAttr attr, size_t word_sz, bool cause_pinned) { assert(_g1h->is_in_cset(old), "Object " PTR_FORMAT " should be in the CSet", p2i(old)); oop forward_ptr = old->forward_to_self_atomic(m, memory_order_relaxed); @@ -635,16 +644,16 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz // evacuation failure recovery. _g1h->mark_evac_failure_object(_worker_id, old, word_sz); - ContinuationGCSupport::transform_stack_chunk(old); - _evacuation_failed_info.register_copy_failure(word_sz); - // For iterating objects that failed evacuation currently we can reuse the - // existing closure to scan evacuated objects; since we are iterating from a - // collection set region (i.e. never a Survivor region), we always need to - // gather cards for this case. - G1SkipCardEnqueueSetter x(&_scanner, false /* skip_card_enqueue */); - old->oop_iterate_backwards(&_scanner); + { + // For iterating objects that failed evacuation currently we can reuse the + // existing closure to scan evacuated objects; since we are iterating from a + // collection set region (i.e. never a Survivor region), we always need to + // gather cards for this case. + G1SkipCardEnqueueSetter x(&_scanner, false /* skip_card_enqueue */); + do_iterate_object(old, old, klass, attr, attr, m.age()); + } return old; } else { diff --git a/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp b/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp index eafd4c22d111e..94ca0878df8e5 100644 --- a/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp +++ b/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp @@ -168,7 +168,7 @@ class G1ParScanThreadState : public CHeapObj { private: void do_partial_array(PartialArrayState* state, bool stolen); - void start_partial_objarray(G1HeapRegionAttr dest_dir, oop from, oop to); + void start_partial_objarray(oop from, oop to); HeapWord* allocate_copy_slow(G1HeapRegionAttr* dest_attr, Klass* klass, @@ -183,6 +183,12 @@ class G1ParScanThreadState : public CHeapObj { void update_bot_after_copying(oop obj, size_t word_sz); + void do_iterate_object(oop const obj, + oop const old, + Klass* const klass, + G1HeapRegionAttr const region_attr, + G1HeapRegionAttr const dest_attr, + uint age); oop do_copy_to_survivor_space(G1HeapRegionAttr region_attr, oop obj, markWord old_mark); @@ -230,7 +236,7 @@ class G1ParScanThreadState : public CHeapObj { void record_evacuation_failed_region(G1HeapRegion* r, uint worker_id, bool cause_pinned); // An attempt to evacuate "obj" has failed; take necessary steps. - oop handle_evacuation_failure_par(oop obj, markWord m, size_t word_sz, bool cause_pinned); + oop handle_evacuation_failure_par(oop obj, markWord m, Klass* klass, G1HeapRegionAttr attr, size_t word_sz, bool cause_pinned); template inline void remember_root_into_optional_region(T* p); diff --git a/test/hotspot/jtreg/gc/g1/TestAllocationFailure.java b/test/hotspot/jtreg/gc/g1/TestAllocationFailure.java index 30bea28b882cd..cd4c991de263c 100644 --- a/test/hotspot/jtreg/gc/g1/TestAllocationFailure.java +++ b/test/hotspot/jtreg/gc/g1/TestAllocationFailure.java @@ -59,7 +59,6 @@ public static void main(String[] args) throws Exception { } static class GCTestWithAllocationFailure { - private static byte[] garbage; private static byte[] largeObject; private static Object[] holder = new Object[200]; // Must be larger than G1GCAllocationFailureALotCount @@ -70,7 +69,7 @@ public static void main(String [] args) { // (Heap size is 32M, we use 17MB for the large object above) // which is larger than G1GCAllocationFailureALotInterval. for (int i = 0; i < 16 * 1024; i++) { - holder[i % holder.length] = new byte[1024]; + holder[i % holder.length] = new Object[256]; } System.out.println("Done"); } From 21cc754a5dac766b978bd08977ee8f7b0d45d0c8 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Thu, 3 Apr 2025 13:27:01 +0200 Subject: [PATCH 2/2] * some additional assert to make sure the scanner is initialized correctly. --- src/hotspot/share/gc/g1/g1OopClosures.hpp | 4 ++++ src/hotspot/share/gc/g1/g1ParScanThreadState.cpp | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/g1/g1OopClosures.hpp b/src/hotspot/share/gc/g1/g1OopClosures.hpp index 965df8ea82a30..3bff668bcec9f 100644 --- a/src/hotspot/share/gc/g1/g1OopClosures.hpp +++ b/src/hotspot/share/gc/g1/g1OopClosures.hpp @@ -107,6 +107,10 @@ class G1ScanEvacuatedObjClosure : public G1ScanClosureBase { void set_ref_discoverer(ReferenceDiscoverer* rd) { set_ref_discoverer_internal(rd); } + +#ifdef ASSERT + bool skip_card_enqueue_set() const { return _skip_card_enqueue != Uninitialized; } +#endif }; // RAII object to properly set the _skip_card_enqueue field in G1ScanEvacuatedObjClosure. diff --git a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp index 04fd343a3ef49..89d7e44e6b3d3 100644 --- a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp +++ b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp @@ -250,6 +250,7 @@ void G1ParScanThreadState::start_partial_objarray(oop from_obj, // The source array is unused when processing states. _partial_array_splitter.start(_task_queue, nullptr, to_array, array_length); + assert(_scanner.skip_card_enqueue_set(), "must be"); // Process the initial chunk. No need to process the type in the // klass, as it will already be handled by processing the built-in // module. @@ -450,6 +451,7 @@ void G1ParScanThreadState::do_iterate_object(oop const obj, _string_dedup_requests.add(old); } + assert(_scanner.skip_card_enqueue_set(), "must be"); obj->oop_iterate_backwards(&_scanner, klass); } @@ -545,7 +547,6 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio // equal: successfully allocated young regions must be survivor regions. assert(dest_attr.is_young() == _g1h->heap_region_containing(obj)->is_survivor(), "must be"); G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_young()); - do_iterate_object(obj, old, klass, region_attr, dest_attr, age); }