Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@ void ShenandoahPassiveMode::initialize_flags() const {
SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahCloneBarrier);
SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahStackWatermarkBarrier);
SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahCardBarrier);

// Final configuration checks
// Passive mode does not instantiate the machinery to support the card table.
// Exit if the flag has been explicitly set.
SHENANDOAH_CHECK_FLAG_UNSET(ShenandoahCardBarrier);
}

ShenandoahHeuristics* ShenandoahPassiveMode::initialize_heuristics(ShenandoahSpaceInfo* space_info) const {
Expand Down
6 changes: 6 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ void ShenandoahDegenGC::op_degenerated() {
// some phase, we have to upgrade the Degenerate GC to Full GC.
heap->clear_cancelled_gc();

// If it's passive mode with ShenandoahCardBarrier turned on: clean the write table
// without swapping the tables since no scan happens in passive mode anyway
if (ShenandoahCardBarrier && !heap->mode()->is_generational()) {
heap->old_generation()->card_scan()->mark_write_table_as_clean();
}

#ifdef ASSERT
if (heap->mode()->is_generational()) {
ShenandoahOldGeneration* old_generation = heap->old_generation();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#ifndef SHARE_GC_SHENANDOAH_SHENANDOAHGENERATIONALHEAP
#define SHARE_GC_SHENANDOAH_SHENANDOAHGENERATIONALHEAP

#include "gc/shenandoah/shenandoahAsserts.hpp"
#include "gc/shenandoah/shenandoahHeap.hpp"
#include "memory/universe.hpp"
#include "utilities/checkedCast.hpp"
Expand All @@ -44,13 +43,13 @@ class ShenandoahGenerationalHeap : public ShenandoahHeap {
void initialize_heuristics() override;

static ShenandoahGenerationalHeap* heap() {
shenandoah_assert_generational();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put these assertions back? I don't see any code in the PR that would invalidate these assertions. I also don't see any changes that would instantiate the generational heap for non generational mode, so if there is code trying to use the generational heap, its behavior will be undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would crash at here with the assertion: code. Provided some of the stacktrace below. It's mostly because we enabled card barrier flag and the code just fell through and assumes it's generational.

Would it be more appropriate to change this line to be ShenandoahHeap* heap = ShenandoahHeap::heap();?

Stack: [0x00007f1082bd2000,0x00007f1082cd2000],  sp=0x00007f1082cce370,  free space=1008k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x13e0e90]  NativeStackPrinter::print_stack(outputStream*, char*, int, unsigned char*&, bool
, int)+0x70  (shenandoahGenerationalHeap.hpp:47)
V  [libjvm.so+0x18fd2e9]  VMError::report(outputStream*, bool)+0x1bff  (vmError.cpp:979)
V  [libjvm.so+0x1900d01]  VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, 
unsigned char*, void const*, void const*, char const*, int, unsigned long)+0x8b1  (vmError.cpp:1887)
V  [libjvm.so+0xacf5ca]  report_vm_status_error(char const*, int, char const*, int, char const*)+0x0  (deb
ug.cpp:196)
V  [libjvm.so+0xacf33b]  print_error_for_unit_test(char const*, char const*, __va_list_tag*)+0x0  (debug.c
pp:149)
V  [libjvm.so+0x15bbec1]  ShenandoahAsserts::assert_generational(char const*, int)+0x8b  (shenandoahAssert
s.cpp:532)
V  [libjvm.so+0x15be196]  ShenandoahGenerationalHeap::heap()+0x19  (shenandoahGenerationalHeap.hpp:47)
V  [libjvm.so+0x16993bc]  void card_mark_barrier<narrowOop>(narrowOop*, oopDesc*)+0x73  (shenandoahReferenceProcessor.cpp:65)
V  [libjvm.so+0x169a2ac]  bool ShenandoahReferenceProcessor::discover<narrowOop>(oopDesc*, ReferenceType, unsigned int)+0x1da  (shenandoahReferenceProcessor.cpp:409)
V  [libjvm.so+0x1698a5d]  ShenandoahReferenceProcessor::discover_reference(oopDesc*, ReferenceType)+0xed  (shenandoahReferenceProcessor.cpp:433)
V  [libjvm.so+0x1684aeb]  bool InstanceRefKlass::try_discover<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >(oopDesc*, ReferenceType, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*)+0x77  (instanceRefKlass.inline.hpp:74)
V  [libjvm.so+0x1682bfc]  void InstanceRefKlass::oop_oop_iterate_discovery<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>, AlwaysContains>(oopDesc*, ReferenceType, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, AlwaysContains&)+0x2c  (instanceRefKlass.inline.hpp:84)
V  [libjvm.so+0x1680acc]  void InstanceRefKlass::oop_oop_iterate_ref_processing<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>, AlwaysContains>(oopDesc*, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, AlwaysContains&)+0x80  (instanceRefKlass.inline.hpp:111)
V  [libjvm.so+0x167ec2e]  void InstanceRefKlass::oop_oop_iterate_ref_processing<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >(oopDesc*, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*)+0x2c  (instanceRefKlass.inline.hpp:134)
V  [libjvm.so+0x167ba88]  void InstanceRefKlass::oop_oop_iterate<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >(oopDesc*, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*)+0x16c  (instanceRefKlass.inline.hpp:154)

assert(ShenandoahCardBarrier, "Should have card barrier to use genenrational heap");
CollectedHeap* heap = Universe::heap();
return cast(heap);
}

static ShenandoahGenerationalHeap* cast(CollectedHeap* heap) {
shenandoah_assert_generational();
assert(ShenandoahCardBarrier, "Should have card barrier to use genenrational heap");
return checked_cast<ShenandoahGenerationalHeap*>(heap);
}

Expand Down
7 changes: 7 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,13 @@ jint ShenandoahHeap::initialize() {
// Now we know the number of regions and heap sizes, initialize the heuristics.
initialize_heuristics();

// If ShenandoahCardBarrier is enabled but it's not generational mode
// it means we're under passive mode and we have to initialize old gen
// for the purpose of having card table.
if (ShenandoahCardBarrier && !(mode()->is_generational())) {
_old_generation = new ShenandoahOldGeneration(max_workers(), max_capacity());
}

assert(_heap_region.byte_size() == heap_rs.size(), "Need to know reserved size for card table");

//
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ class ShenandoahHeap : public CollectedHeap {
}

ShenandoahOldGeneration* old_generation() const {
assert(mode()->is_generational(), "Old generation requires generational mode");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assert that ShenandoahCardBarrier is on instead of removing this assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

assert(ShenandoahCardBarrier, "Card mark barrier should be on");
return _old_generation;
}

Expand Down
16 changes: 16 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,18 @@ void ShenandoahDirectCardMarkRememberedSet::mark_read_table_as_clean() {
log_develop_debug(gc, barrier)("Cleaned read_table from " PTR_FORMAT " to " PTR_FORMAT, p2i(&(read_table[0])), p2i(end_bp));
}

void ShenandoahDirectCardMarkRememberedSet::mark_write_table_as_clean() {
CardValue* write_table = _card_table->write_byte_map();
CardValue* bp = &(write_table)[0];
CardValue* end_bp = &(write_table)[_card_table->last_valid_index()];

while (bp <= end_bp) {
*bp++ = CardTable::clean_card_val();
}

log_develop_debug(gc, barrier)("Cleaned write_table from " PTR_FORMAT " to " PTR_FORMAT, p2i(&(write_table[0])), p2i(end_bp));
}

// No lock required because arguments align with card boundaries.
void ShenandoahCardCluster::reset_object_range(HeapWord* from, HeapWord* to) {
assert(((((unsigned long long) from) & (CardTable::card_size() - 1)) == 0) &&
Expand Down Expand Up @@ -330,6 +342,10 @@ void ShenandoahScanRemembered::mark_read_table_as_clean() {
_rs->mark_read_table_as_clean();
}

void ShenandoahScanRemembered::mark_write_table_as_clean() {
_rs->mark_write_table_as_clean();
}

void ShenandoahScanRemembered::reset_object_range(HeapWord* from, HeapWord* to) {
_scc->reset_object_range(from, to);
}
Expand Down
4 changes: 4 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,8 @@ class ShenandoahDirectCardMarkRememberedSet: public CHeapObj<mtGC> {
// See comment in ShenandoahScanRemembered
inline void mark_read_table_as_clean();

inline void mark_write_table_as_clean();

// Merge any dirty values from write table into the read table, while leaving
// the write table unchanged.
void merge_write_table(HeapWord* start, size_t word_count);
Expand Down Expand Up @@ -769,6 +771,8 @@ class ShenandoahScanRemembered: public CHeapObj<mtGC> {
// the "write" table.
void mark_read_table_as_clean();

void mark_write_table_as_clean();

// Swaps read and write card tables pointers in effect setting a clean card
// table for the next GC cycle.
void swap_card_tables() { _rs->swap_card_tables(); }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright Amazon.com Inc. 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
* @summary Test passive mode with card barrier with a gc heavy app. A simple hello world in TestSelectiveBarrierFlags
* does not always surface crashes
* @requires vm.gc.Shenandoah
* @library /test/lib
*
* @run main/othervm -XX:+UnlockExperimentalVMOptions -XX:+UnlockDiagnosticVMOptions -XX:+UseShenandoahGC -Xmx128m -XX:ShenandoahGCMode=passive -XX:+ShenandoahCardBarrier TestPassiveModeWithCardBarrier
*/

import java.util.*;
import java.util.concurrent.*;

import jdk.test.lib.process.ProcessTools;
import jdk.test.lib.process.OutputAnalyzer;

public class TestPassiveModeWithCardBarrier {
public static void main(String[] args) throws Exception {
List<byte[]> junk = new ArrayList<>();
int junkLength = 1000;
int totalRounds = 10;
int round = 0;

while (round++ < totalRounds) {
for (int i = 0; i < junkLength; i++) {
junk.add(new byte[1024]);
}

System.out.println(junk.hashCode());
}

// trigger a full gc in case it was all degen
System.gc();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,30 @@
public class TestWrongBarrierEnable {

public static void main(String[] args) throws Exception {
String[] concurrent = { "ShenandoahSATBBarrier" };
String[] concurrent = {
"ShenandoahLoadRefBarrier",
"ShenandoahSATBBarrier",
"ShenandoahCASBarrier",
"ShenandoahCloneBarrier",
"ShenandoahStackWatermarkBarrier",
};
String[] generational = { "ShenandoahCardBarrier" };
String[] all = { "ShenandoahSATBBarrier", "ShenandoahCardBarrier" };
String[] all = {
"ShenandoahLoadRefBarrier",
"ShenandoahSATBBarrier",
"ShenandoahCASBarrier",
"ShenandoahCloneBarrier",
"ShenandoahStackWatermarkBarrier",
"ShenandoahCardBarrier"
};

shouldPassAll("-XX:ShenandoahGCHeuristics=adaptive", concurrent);
shouldPassAll("-XX:ShenandoahGCHeuristics=static", concurrent);
shouldPassAll("-XX:ShenandoahGCHeuristics=compact", concurrent);
shouldPassAll("-XX:ShenandoahGCHeuristics=aggressive", concurrent);
shouldPassAll("-XX:ShenandoahGCMode=passive", concurrent);
shouldPassAll("-XX:ShenandoahGCMode=passive", all);
shouldPassAll("-XX:ShenandoahGCMode=generational", all);
shouldFailAll("-XX:ShenandoahGCMode=satb", generational);
shouldFailAll("-XX:ShenandoahGCMode=passive", generational);
}

private static void shouldFailAll(String h, String[] barriers) throws Exception {
Expand Down