RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u - the review thread
Aditya Mandaleeka
adityam at microsoft.com
Fri Jul 24 02:43:05 UTC 2020
Hi Roman,
I took a first pass at reviewing the shared code changes that are not under test/, with an eye toward
ways in which the patch could affect Shenandoah-less builds. Things mostly looked good, and many of these
are nitpicks.
The review below is based on http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.10-shared/
As a disclaimer, I'm not currently an official reviewer on the project.
Thanks,
Aditya
Here it goes:
============================================================
src/hotspot/share/opto/escape.cpp, ~590:
@@ -554,8 +567,8 @@
// Pointer stores in G1 barriers looks like unsafe access.
// Ignore such stores to be able scalar replace non-escaping
// allocations.
-#if INCLUDE_G1GC
- if (UseG1GC && adr->is_AddP()) {
+#if INCLUDE_G1GC || INCLUDE_SHENANDOAHGC
+ if ((UseG1GC SHENANDOAHGC_ONLY(|| UseShenandoahGC)) && adr->is_AddP()) {
Node* base = get_addp_base(adr);
if (base->Opcode() == Op_LoadP &&
base->in(MemNode::Address)->is_AddP()) {
@@ -563,7 +576,15 @@
Node* tls = get_addp_base(adr);
if (tls->Opcode() == Op_ThreadLocal) {
int offs = (int)igvn->find_intptr_t_con(adr->in(AddPNode::Offset), Type::OffsetBot);
- if (offs == in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset())) {
+#if INCLUDE_G1GC && INCLUDE_SHENANDOAHGC
+ const int buf_offset = in_bytes(UseG1GC ? G1ThreadLocalData::satb_mark_queue_buffer_offset()
+ : ShenandoahThreadLocalData::satb_mark_queue_buffer_offset());
+#elif INCLUDE_G1GC
+ const int buf_offset = in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset());
+#else
+ const int buf_offset = in_bytes(ShenandoahThreadLocalData::satb_mark_queue_buffer_offset());
+#endif
+ if (offs == buf_offset) {
break; // G1 pre barrier previous oop value store.
}
if (offs == in_bytes(G1ThreadLocalData::dirty_card_queue_buffer_offset())) {
-----
The outermost block has been relaxed to be under `#if INCLUDE_G1GC || INCLUDE_SHENANDOAHGC`, but I think
that last line above (the call to G1ThreadLocalData::dirty_card_queue_buffer_offset()) won't compile if G1
is disabled, will it? I haven't verified it myself, but you may want to check that. I assume including
Shenandoah and not G1 is a valid configuration based on the other checks you added.
============================================================
src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp, ~681:
#ifdef ASSERT
@@ -674,6 +675,11 @@
if (type == T_OBJECT || type == T_ARRAY) {
cmp_value.load_item_force(FrameMap::rax_oop_opr);
new_value.load_item();
+#if INCLUDE_SHENANDOAHGC
+ if (UseShenandoahGC) {
+ __ cas_obj(addr->as_address_ptr()->base(), cmp_value.result(), new_value.result(), new_register(T_OBJECT), new_register(T_OBJECT));
+ } else
+#endif
__ cas_obj(addr->as_address_ptr()->base(), cmp_value.result(), new_value.result(), ill, ill);
} else if (type == T_INT) {
-----
The hanging brace-less else scares me a bit since someone might add something else after the
endif and unknowingly cause problems for Shenandoah builds.
============================================================
src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp, ~708:
@@ -699,6 +705,12 @@
// Because we want a 2-arg form of xchg and xadd
__ move(value.result(), result);
assert(type == T_INT || is_oop LP64_ONLY( || type == T_LONG ), "unexpected type");
+#if INCLUDE_SHENANDOAHGC
+ if (UseShenandoahGC) {
+ LIR_Opr tmp = is_oop ? new_register(type) : LIR_OprFact::illegalOpr;
+ __ xchg(addr, result, result, tmp);
+ } else
+#endif
__ xchg(addr, result, result, LIR_OprFact::illegalOpr);
return result;
}
-----
This is another case like the above (and in the same file).
I understand there's a motovation to keep the non-Shenandoah code delta minimal, but something like this seems much nicer:
LIR_Opr tmp = LIR_OprFact::illegalOpr;
#if INCLUDE_SHENANDOAHGC
if (UseShenandoahGC && is_oop) {
tmp = new_register(type);
#endif
__ xchg(addr, result, result, tmp);
return result;
============================================================
src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp, two places
@@ -2234,6 +2245,14 @@
switch (in_sig_bt[i]) {
case T_ARRAY:
if (is_critical_native) {
+#if INCLUDE_SHENANDOAHGC
+ // pin before unpack
+ if (UseShenandoahGC) {
+ assert(pinned_slot <= stack_slots, "overflow");
+ ShenandoahBarrierSet::assembler()->pin_critical_native_array(masm, in_regs[i], pinned_slot);
+ pinned_args.append(i);
+ }
+#endif
@@ -2450,6 +2469,22 @@
default : ShouldNotReachHere();
}
+#if INCLUDE_SHENANDOAHGC
+ if (UseShenandoahGC) {
+ // unpin pinned arguments
+ pinned_slot = oop_handle_offset;
+ if (pinned_args.length() > 0) {
+ // save return value that may be overwritten otherwise.
+ save_native_result(masm, ret_type, stack_slots);
+ for (int index = 0; index < pinned_args.length(); index ++) {
+ int i = pinned_args.at(index);
+ assert(pinned_slot <= stack_slots, "overflow");
+ ShenandoahBarrierSet::assembler()->unpin_critical_native_array(masm, in_regs[i], pinned_slot);
+ }
+ restore_native_result(masm, ret_type, stack_slots);
+ }
+ }
+#endif
-----
Minor issue: There are tab characters used in indenting the ShenandoahBarrierSet* lines, inconsistent
with the surrounding code.
============================================================
Various files
- * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
-----
What's the normal policy around how these years are updated? If the change is being made now, should
they be 2020?
============================================================
src/hotspot/share/gc/shared/gcConfig.cpp, ~60
+ CMSGC_ONLY(static CMSArguments cmsArguments;)
+ EPSILONGC_ONLY(static EpsilonArguments epsilonArguments;)
+ G1GC_ONLY(static G1Arguments g1Arguments;)
+ PARALLELGC_ONLY(static ParallelArguments parallelArguments;)
+ SERIALGC_ONLY(static SerialArguments serialArguments;)
+SHENANDOAHGC_ONLY(static ShenandoahArguments shenandoahArguments;)
+ ZGC_ONLY(static ZArguments zArguments;)
-----
Minor issue: alignment of the others needs to be adjusted
============================================================
src/hotspot/share/gc/shared/gcConfig.cpp, ~98
@@ -90,14 +95,14 @@
bool GCConfig::_gc_selected_ergonomically = false;
void GCConfig::fail_if_unsupported_gc_is_selected() {
- NOT_CMSGC( FAIL_IF_SELECTED(UseConcMarkSweepGC, true));
- NOT_EPSILONGC( FAIL_IF_SELECTED(UseEpsilonGC, true));
- NOT_G1GC( FAIL_IF_SELECTED(UseG1GC, true));
- NOT_PARALLELGC(FAIL_IF_SELECTED(UseParallelGC, true));
- NOT_PARALLELGC(FAIL_IF_SELECTED(UseParallelOldGC, true));
- NOT_SERIALGC( FAIL_IF_SELECTED(UseSerialGC, true));
- NOT_SERIALGC( FAIL_IF_SELECTED(UseParallelOldGC, false));
- NOT_ZGC( FAIL_IF_SELECTED(UseZGC, true));
+ NOT_CMSGC( FAIL_IF_SELECTED(UseConcMarkSweepGC, true));
+ NOT_EPSILONGC( FAIL_IF_SELECTED(UseEpsilonGC, true));
+ NOT_G1GC( FAIL_IF_SELECTED(UseG1GC, true));
+ NOT_PARALLELGC( FAIL_IF_SELECTED(UseParallelGC, true));
+ NOT_PARALLELGC( FAIL_IF_SELECTED(UseParallelOldGC, true));
+ NOT_SERIALGC( FAIL_IF_SELECTED(UseSerialGC, true));
+ NOT_SERIALGC( FAIL_IF_SELECTED(UseParallelOldGC, false));
+ NOT_ZGC( FAIL_IF_SELECTED(UseZGC, true));
-----
Minor issue: These whitespace changes can be undone. (I guess a prior version had a Shenandoah thing
added here which has now been removed?)
============================================================
src/hotspot/share/runtime/stackValue.cpp, ~114
- // Decode narrowoop and wrap a handle around the oop
- Handle h(Thread::current(), CompressedOops::decode(value.noop));
+ // Decode narrowoop
+ oop val = CompressedOops::decode(value.noop);
+ // Deoptimization must make sure all oops have passed load barriers
+#if INCLUDE_SHENANDOAHGC
+ if (UseShenandoahGC) {
+ val = ShenandoahBarrierSet::barrier_set()->load_reference_barrier(val);
+ }
+#endif
+ Handle h(Thread::current(), val); // Wrap a handle around the oop
-----
I'd move the load barrier comment into the `if (UseShenandoahGC)` for better readability.
============================================================
src/hotspot/share/gc/shared/vmStructs_gc.hpp, three places
SERIALGC_ONLY(VM_STRUCTS_SERIALGC(nonstatic_field, \
volatile_nonstatic_field, \
static_field)) \
+ SHENANDOAHGC_ONLY(VM_STRUCTS_SHENANDOAH(nonstatic_field, \
+ volatile_nonstatic_field, \
+ static_field))
-----
Ultra nit-pick: The others have the second and third arguments aligned with the first. There
are three places in this file where you've added the Shenandoah ones and they're not aligned
this way. Hey, I warned you some of these comments are minor :).
More information about the jdk-updates-dev
mailing list