Skip to content

Commit c50a0a1

Browse files
author
Thomas Schatzl
committed
8352508: [Redo] G1: Pinned regions with pinned objects only reachable by native code crash VM
Reviewed-by: ayang, iwalulya
1 parent 89e5c42 commit c50a0a1

File tree

6 files changed

+156
-15
lines changed

6 files changed

+156
-15
lines changed

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1273,7 +1273,8 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask {
12731273
// The liveness of this humongous obj decided by either its allocation
12741274
// time (allocated after conc-mark-start, i.e. live) or conc-marking.
12751275
const bool is_live = _cm->top_at_mark_start(hr) == hr->bottom()
1276-
|| _cm->contains_live_object(hr->hrm_index());
1276+
|| _cm->contains_live_object(hr->hrm_index())
1277+
|| hr->has_pinned_objects();
12771278
if (is_live) {
12781279
const bool selected_for_rebuild = tracker->update_humongous_before_rebuild(hr);
12791280
auto on_humongous_region = [&] (G1HeapRegion* hr) {
@@ -1291,7 +1292,9 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask {
12911292
uint region_idx = hr->hrm_index();
12921293
hr->note_end_of_marking(_cm->top_at_mark_start(hr), _cm->live_bytes(region_idx), _cm->incoming_refs(region_idx));
12931294

1294-
if (hr->live_bytes() != 0) {
1295+
const bool is_live = hr->live_bytes() != 0
1296+
|| hr->has_pinned_objects();
1297+
if (is_live) {
12951298
if (tracker->update_old_before_rebuild(hr)) {
12961299
_num_selected_for_rebuild++;
12971300
}

src/hotspot/share/gc/g1/g1ParScanThreadState.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,12 @@ void G1ParScanThreadStateSet::record_unused_optional_region(G1HeapRegion* hr) {
614614
}
615615
}
616616

617+
void G1ParScanThreadState::record_evacuation_failed_region(G1HeapRegion* r, uint worker_id, bool cause_pinned) {
618+
if (_evac_failure_regions->record(worker_id, r->hrm_index(), cause_pinned)) {
619+
G1HeapRegionPrinter::evac_failure(r);
620+
}
621+
}
622+
617623
NOINLINE
618624
oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, size_t word_sz, bool cause_pinned) {
619625
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
623629
// Forward-to-self succeeded. We are the "owner" of the object.
624630
G1HeapRegion* r = _g1h->heap_region_containing(old);
625631

626-
if (_evac_failure_regions->record(_worker_id, r->hrm_index(), cause_pinned)) {
627-
G1HeapRegionPrinter::evac_failure(r);
628-
}
632+
record_evacuation_failed_region(r, _worker_id, cause_pinned);
629633

630634
// Mark the failing object in the marking bitmap and later use the bitmap to handle
631635
// evacuation failure recovery.

src/hotspot/share/gc/g1/g1ParScanThreadState.hpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
228228
Tickspan trim_ticks() const;
229229
void reset_trim_ticks();
230230

231+
void record_evacuation_failed_region(G1HeapRegion* r, uint worker_id, bool cause_pinned);
231232
// An attempt to evacuate "obj" has failed; take necessary steps.
232233
oop handle_evacuation_failure_par(oop obj, markWord m, size_t word_sz, bool cause_pinned);
233234

src/hotspot/share/gc/g1/g1Policy.cpp

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -660,13 +660,6 @@ void G1Policy::record_concurrent_refinement_stats(size_t pending_cards,
660660

661661
bool G1Policy::should_retain_evac_failed_region(uint index) const {
662662
size_t live_bytes = _g1h->region_at(index)->live_bytes();
663-
664-
#ifdef ASSERT
665-
G1HeapRegion* r = _g1h->region_at(index);
666-
assert(live_bytes != 0,
667-
"live bytes not set for %u used %zu garbage %zu cm-live %zu pinned %d",
668-
index, r->used(), r->garbage_bytes(), live_bytes, r->has_pinned_objects());
669-
#endif
670663
size_t threshold = G1RetainRegionLiveThresholdPercent * G1HeapRegion::GrainBytes / 100;
671664
return live_bytes < threshold;
672665
}

src/hotspot/share/gc/g1/g1YoungCollector.cpp

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -587,15 +587,37 @@ class G1ParEvacuateFollowersClosure : public VoidClosure {
587587
};
588588

589589
class G1EvacuateRegionsBaseTask : public WorkerTask {
590+
591+
// All pinned regions in the collection set must be registered as failed
592+
// regions as there is no guarantee that there is a reference reachable by
593+
// Java code (i.e. only by native code) that adds it to the evacuation failed
594+
// regions.
595+
void record_pinned_regions(G1ParScanThreadState* pss, uint worker_id) {
596+
class RecordPinnedRegionClosure : public G1HeapRegionClosure {
597+
G1ParScanThreadState* _pss;
598+
uint _worker_id;
599+
600+
public:
601+
RecordPinnedRegionClosure(G1ParScanThreadState* pss, uint worker_id) : _pss(pss), _worker_id(worker_id) { }
602+
603+
bool do_heap_region(G1HeapRegion* r) {
604+
if (r->has_pinned_objects()) {
605+
_pss->record_evacuation_failed_region(r, _worker_id, true /* cause_pinned */);
606+
}
607+
return false;
608+
}
609+
} cl(pss, worker_id);
610+
611+
_g1h->collection_set_iterate_increment_from(&cl, worker_id);
612+
}
613+
590614
protected:
591615
G1CollectedHeap* _g1h;
592616
G1ParScanThreadStateSet* _per_thread_states;
593617

594618
G1ScannerTasksQueueSet* _task_queues;
595619
TaskTerminator _terminator;
596620

597-
uint _num_workers;
598-
599621
void evacuate_live_objects(G1ParScanThreadState* pss,
600622
uint worker_id,
601623
G1GCPhaseTimes::GCParPhases objcopy_phase,
@@ -631,6 +653,9 @@ class G1EvacuateRegionsBaseTask : public WorkerTask {
631653

632654
virtual void evacuate_live_objects(G1ParScanThreadState* pss, uint worker_id) = 0;
633655

656+
private:
657+
volatile bool _pinned_regions_recorded;
658+
634659
public:
635660
G1EvacuateRegionsBaseTask(const char* name,
636661
G1ParScanThreadStateSet* per_thread_states,
@@ -641,7 +666,7 @@ class G1EvacuateRegionsBaseTask : public WorkerTask {
641666
_per_thread_states(per_thread_states),
642667
_task_queues(task_queues),
643668
_terminator(num_workers, _task_queues),
644-
_num_workers(num_workers)
669+
_pinned_regions_recorded(false)
645670
{ }
646671

647672
void work(uint worker_id) {
@@ -653,6 +678,9 @@ class G1EvacuateRegionsBaseTask : public WorkerTask {
653678
G1ParScanThreadState* pss = _per_thread_states->state_for_worker(worker_id);
654679
pss->set_ref_discoverer(_g1h->ref_processor_stw());
655680

681+
if (!Atomic::cmpxchg(&_pinned_regions_recorded, false, true)) {
682+
record_pinned_regions(pss, worker_id);
683+
}
656684
scan_roots(pss, worker_id);
657685
evacuate_live_objects(pss, worker_id);
658686
}
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/*
2+
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/* @test
25+
* @bug 8351921 8352508
26+
* @summary Test that pinned regions with no Java references into them
27+
* do not make the garbage collector reclaim that region.
28+
* This test simulates this behavior using Whitebox methods
29+
* to pin a Java object in a region with no other pinnable objects and
30+
* lose the reference to it before the garbage collection.
31+
* @requires vm.gc.G1
32+
* @requires vm.debug
33+
* @library /test/lib /
34+
* @modules java.management
35+
* @build jdk.test.whitebox.WhiteBox
36+
* @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
37+
* @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseG1GC
38+
* -Xbootclasspath/a:. -Xlog:gc=debug,gc+ergo+cset=trace,gc+phases=debug -XX:G1HeapRegionSize=1m -Xms30m gc.g1.pinnedobjs.TestPinnedEvacEmpty regular
39+
*
40+
* @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseG1GC
41+
* -Xbootclasspath/a:. -Xlog:gc=debug,gc+ergo+cset=trace,gc+phases=debug -XX:G1HeapRegionSize=1m -Xms30m gc.g1.pinnedobjs.TestPinnedEvacEmpty humongous
42+
*/
43+
44+
package gc.g1.pinnedobjs;
45+
46+
import gc.testlibrary.Helpers;
47+
48+
import jdk.test.lib.Asserts;
49+
import jdk.test.lib.process.OutputAnalyzer;
50+
import jdk.test.lib.process.ProcessTools;
51+
import jdk.test.whitebox.WhiteBox;
52+
53+
public class TestPinnedEvacEmpty {
54+
55+
private static final WhiteBox wb = WhiteBox.getWhiteBox();
56+
57+
private static final int objSize = (int)wb.getObjectSize(new Object());
58+
59+
private static Object allocHumongous() {
60+
final int M = 1024 * 1024;
61+
// The target object to pin. Since it is humongous, it will always be in its
62+
// own regions.
63+
return new int[M];
64+
}
65+
66+
private static Object allocRegular() {
67+
// How many j.l.Object should we allocate when creating garbage.
68+
final int numAllocations = 1024 * 1024 * 3 / objSize;
69+
70+
// Allocate garbage so that the target object will be in a new region.
71+
for (int i = 0; i < numAllocations; i++) {
72+
Object z = new Object();
73+
}
74+
int[] o = new int[100]; // The target object to pin.
75+
// Further allocate garbage so that any additional allocations of potentially
76+
// pinned objects can not be allocated in the same region as the target object.
77+
for (int i = 0; i < numAllocations; i++) {
78+
Object z = new Object();
79+
}
80+
81+
Asserts.assertTrue(!wb.isObjectInOldGen(o), "should not be in old gen already");
82+
return o;
83+
}
84+
85+
public static void main(String[] args) throws Exception {
86+
System.out.println("Testing " + args[0] + " objects...");
87+
88+
// Remove garbage from VM initialization.
89+
wb.fullGC();
90+
91+
// Allocate target object according to arguments.
92+
Object o = args[0].equals("regular") ? allocRegular() : allocHumongous();
93+
94+
// Pin the object.
95+
wb.pinObject(o);
96+
97+
// And forget the (Java) reference to the int array. After this, the garbage
98+
// collection should find a completely empty pinned region. The collector
99+
// must not collect/free it.
100+
o = null;
101+
102+
// Full collection should not crash the VM in case of "empty" pinned regions.
103+
wb.fullGC();
104+
105+
// Do a young garbage collection to zap the data in the pinned region. This test
106+
// achieves that by executing a concurrent cycle that both performs both a young
107+
// garbage collection as well as checks that errorneous reclamation does not occur
108+
// in the Remark pause.
109+
wb.g1RunConcurrentGC();
110+
System.out.println("Done");
111+
}
112+
}

0 commit comments

Comments
 (0)