From c50a0a1fc126a67528448b282bcfc375abfac142 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Thu, 27 Mar 2025 14:30:31 +0000 Subject: [PATCH] 8352508: [Redo] G1: Pinned regions with pinned objects only reachable by native code crash VM Reviewed-by: ayang, iwalulya --- src/hotspot/share/gc/g1/g1ConcurrentMark.cpp | 7 +- .../share/gc/g1/g1ParScanThreadState.cpp | 10 +- .../share/gc/g1/g1ParScanThreadState.hpp | 1 + src/hotspot/share/gc/g1/g1Policy.cpp | 7 -- src/hotspot/share/gc/g1/g1YoungCollector.cpp | 34 +++++- .../gc/g1/pinnedobjs/TestPinnedEvacEmpty.java | 112 ++++++++++++++++++ 6 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 test/hotspot/jtreg/gc/g1/pinnedobjs/TestPinnedEvacEmpty.java diff --git a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp index 41878a20935..0b8e66f9043 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp @@ -1273,7 +1273,8 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask { // The liveness of this humongous obj decided by either its allocation // time (allocated after conc-mark-start, i.e. live) or conc-marking. const bool is_live = _cm->top_at_mark_start(hr) == hr->bottom() - || _cm->contains_live_object(hr->hrm_index()); + || _cm->contains_live_object(hr->hrm_index()) + || hr->has_pinned_objects(); if (is_live) { const bool selected_for_rebuild = tracker->update_humongous_before_rebuild(hr); auto on_humongous_region = [&] (G1HeapRegion* hr) { @@ -1291,7 +1292,9 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask { uint region_idx = hr->hrm_index(); hr->note_end_of_marking(_cm->top_at_mark_start(hr), _cm->live_bytes(region_idx), _cm->incoming_refs(region_idx)); - if (hr->live_bytes() != 0) { + const bool is_live = hr->live_bytes() != 0 + || hr->has_pinned_objects(); + if (is_live) { if (tracker->update_old_before_rebuild(hr)) { _num_selected_for_rebuild++; } diff --git a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp index e8da6c50d87..298222b35b5 100644 --- a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp +++ b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp @@ -614,6 +614,12 @@ void G1ParScanThreadStateSet::record_unused_optional_region(G1HeapRegion* hr) { } } +void G1ParScanThreadState::record_evacuation_failed_region(G1HeapRegion* r, uint worker_id, bool cause_pinned) { + if (_evac_failure_regions->record(worker_id, r->hrm_index(), cause_pinned)) { + G1HeapRegionPrinter::evac_failure(r); + } +} + NOINLINE oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, size_t word_sz, bool cause_pinned) { assert(_g1h->is_in_cset(old), "Object " PTR_FORMAT " should be in the CSet", p2i(old)); @@ -623,9 +629,7 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz // Forward-to-self succeeded. We are the "owner" of the object. G1HeapRegion* r = _g1h->heap_region_containing(old); - if (_evac_failure_regions->record(_worker_id, r->hrm_index(), cause_pinned)) { - G1HeapRegionPrinter::evac_failure(r); - } + record_evacuation_failed_region(r, _worker_id, cause_pinned); // Mark the failing object in the marking bitmap and later use the bitmap to handle // evacuation failure recovery. diff --git a/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp b/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp index f5dccaee9cf..eafd4c22d11 100644 --- a/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp +++ b/src/hotspot/share/gc/g1/g1ParScanThreadState.hpp @@ -228,6 +228,7 @@ public: Tickspan trim_ticks() const; void reset_trim_ticks(); + 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); diff --git a/src/hotspot/share/gc/g1/g1Policy.cpp b/src/hotspot/share/gc/g1/g1Policy.cpp index 8aaa47ba08f..c20e2c971a8 100644 --- a/src/hotspot/share/gc/g1/g1Policy.cpp +++ b/src/hotspot/share/gc/g1/g1Policy.cpp @@ -660,13 +660,6 @@ void G1Policy::record_concurrent_refinement_stats(size_t pending_cards, bool G1Policy::should_retain_evac_failed_region(uint index) const { size_t live_bytes = _g1h->region_at(index)->live_bytes(); - -#ifdef ASSERT - G1HeapRegion* r = _g1h->region_at(index); - assert(live_bytes != 0, - "live bytes not set for %u used %zu garbage %zu cm-live %zu pinned %d", - index, r->used(), r->garbage_bytes(), live_bytes, r->has_pinned_objects()); -#endif size_t threshold = G1RetainRegionLiveThresholdPercent * G1HeapRegion::GrainBytes / 100; return live_bytes < threshold; } diff --git a/src/hotspot/share/gc/g1/g1YoungCollector.cpp b/src/hotspot/share/gc/g1/g1YoungCollector.cpp index 2223d667998..44803564de1 100644 --- a/src/hotspot/share/gc/g1/g1YoungCollector.cpp +++ b/src/hotspot/share/gc/g1/g1YoungCollector.cpp @@ -587,6 +587,30 @@ public: }; class G1EvacuateRegionsBaseTask : public WorkerTask { + + // All pinned regions in the collection set must be registered as failed + // regions as there is no guarantee that there is a reference reachable by + // Java code (i.e. only by native code) that adds it to the evacuation failed + // regions. + void record_pinned_regions(G1ParScanThreadState* pss, uint worker_id) { + class RecordPinnedRegionClosure : public G1HeapRegionClosure { + G1ParScanThreadState* _pss; + uint _worker_id; + + public: + RecordPinnedRegionClosure(G1ParScanThreadState* pss, uint worker_id) : _pss(pss), _worker_id(worker_id) { } + + bool do_heap_region(G1HeapRegion* r) { + if (r->has_pinned_objects()) { + _pss->record_evacuation_failed_region(r, _worker_id, true /* cause_pinned */); + } + return false; + } + } cl(pss, worker_id); + + _g1h->collection_set_iterate_increment_from(&cl, worker_id); + } + protected: G1CollectedHeap* _g1h; G1ParScanThreadStateSet* _per_thread_states; @@ -594,8 +618,6 @@ protected: G1ScannerTasksQueueSet* _task_queues; TaskTerminator _terminator; - uint _num_workers; - void evacuate_live_objects(G1ParScanThreadState* pss, uint worker_id, G1GCPhaseTimes::GCParPhases objcopy_phase, @@ -631,6 +653,9 @@ protected: virtual void evacuate_live_objects(G1ParScanThreadState* pss, uint worker_id) = 0; +private: + volatile bool _pinned_regions_recorded; + public: G1EvacuateRegionsBaseTask(const char* name, G1ParScanThreadStateSet* per_thread_states, @@ -641,7 +666,7 @@ public: _per_thread_states(per_thread_states), _task_queues(task_queues), _terminator(num_workers, _task_queues), - _num_workers(num_workers) + _pinned_regions_recorded(false) { } void work(uint worker_id) { @@ -653,6 +678,9 @@ public: G1ParScanThreadState* pss = _per_thread_states->state_for_worker(worker_id); pss->set_ref_discoverer(_g1h->ref_processor_stw()); + if (!Atomic::cmpxchg(&_pinned_regions_recorded, false, true)) { + record_pinned_regions(pss, worker_id); + } scan_roots(pss, worker_id); evacuate_live_objects(pss, worker_id); } diff --git a/test/hotspot/jtreg/gc/g1/pinnedobjs/TestPinnedEvacEmpty.java b/test/hotspot/jtreg/gc/g1/pinnedobjs/TestPinnedEvacEmpty.java new file mode 100644 index 00000000000..2b7bc9a5624 --- /dev/null +++ b/test/hotspot/jtreg/gc/g1/pinnedobjs/TestPinnedEvacEmpty.java @@ -0,0 +1,112 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* @test + * @bug 8351921 8352508 + * @summary Test that pinned regions with no Java references into them + * do not make the garbage collector reclaim that region. + * This test simulates this behavior using Whitebox methods + * to pin a Java object in a region with no other pinnable objects and +* lose the reference to it before the garbage collection. + * @requires vm.gc.G1 + * @requires vm.debug + * @library /test/lib / + * @modules java.management + * @build jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseG1GC + * -Xbootclasspath/a:. -Xlog:gc=debug,gc+ergo+cset=trace,gc+phases=debug -XX:G1HeapRegionSize=1m -Xms30m gc.g1.pinnedobjs.TestPinnedEvacEmpty regular + * + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseG1GC + * -Xbootclasspath/a:. -Xlog:gc=debug,gc+ergo+cset=trace,gc+phases=debug -XX:G1HeapRegionSize=1m -Xms30m gc.g1.pinnedobjs.TestPinnedEvacEmpty humongous + */ + +package gc.g1.pinnedobjs; + +import gc.testlibrary.Helpers; + +import jdk.test.lib.Asserts; +import jdk.test.lib.process.OutputAnalyzer; +import jdk.test.lib.process.ProcessTools; +import jdk.test.whitebox.WhiteBox; + +public class TestPinnedEvacEmpty { + + private static final WhiteBox wb = WhiteBox.getWhiteBox(); + + private static final int objSize = (int)wb.getObjectSize(new Object()); + + private static Object allocHumongous() { + final int M = 1024 * 1024; + // The target object to pin. Since it is humongous, it will always be in its + // own regions. + return new int[M]; + } + + private static Object allocRegular() { + // How many j.l.Object should we allocate when creating garbage. + final int numAllocations = 1024 * 1024 * 3 / objSize; + + // Allocate garbage so that the target object will be in a new region. + for (int i = 0; i < numAllocations; i++) { + Object z = new Object(); + } + int[] o = new int[100]; // The target object to pin. + // Further allocate garbage so that any additional allocations of potentially + // pinned objects can not be allocated in the same region as the target object. + for (int i = 0; i < numAllocations; i++) { + Object z = new Object(); + } + + Asserts.assertTrue(!wb.isObjectInOldGen(o), "should not be in old gen already"); + return o; + } + + public static void main(String[] args) throws Exception { + System.out.println("Testing " + args[0] + " objects..."); + + // Remove garbage from VM initialization. + wb.fullGC(); + + // Allocate target object according to arguments. + Object o = args[0].equals("regular") ? allocRegular() : allocHumongous(); + + // Pin the object. + wb.pinObject(o); + + // And forget the (Java) reference to the int array. After this, the garbage + // collection should find a completely empty pinned region. The collector + // must not collect/free it. + o = null; + + // Full collection should not crash the VM in case of "empty" pinned regions. + wb.fullGC(); + + // Do a young garbage collection to zap the data in the pinned region. This test + // achieves that by executing a concurrent cycle that both performs both a young + // garbage collection as well as checks that errorneous reclamation does not occur + // in the Remark pause. + wb.g1RunConcurrentGC(); + System.out.println("Done"); + } +}