From 2525f39d35b7ce556418812613b7cc35d03c1dc6 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 21 Dec 2020 12:42:34 +0000 Subject: [PATCH 1/7] 8258714: Shenandoah: Process references before evacuation during degen Reviewed-by: shade --- .../share/gc/shenandoah/shenandoahHeap.cpp | 16 ++++++++++------ .../share/gc/shenandoah/shenandoahHeap.hpp | 1 + .../gc/shenandoah/shenandoahMarkCompact.cpp | 1 - .../gc/shenandoah/shenandoahPhaseTimings.cpp | 2 ++ .../gc/shenandoah/shenandoahPhaseTimings.hpp | 2 ++ 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index 09d8b53318a..f7d65771034 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -2055,6 +2055,15 @@ void ShenandoahHeap::op_weak_refs() { } } +void ShenandoahHeap::stw_weak_refs(bool full_gc) { + // Weak refs processing + ShenandoahTimingsTracker t(full_gc ? ShenandoahPhaseTimings::full_gc_weakrefs_process + : ShenandoahPhaseTimings::degen_gc_weakrefs_process); + ShenandoahGCWorkerPhase worker_phase(full_gc ? ShenandoahPhaseTimings::full_gc_weakrefs_process + : ShenandoahPhaseTimings::degen_gc_weakrefs_process); + ref_processor()->process_references(workers(), false /* concurrent */); +} + void ShenandoahHeap::op_weak_roots() { if (is_concurrent_weak_root_in_progress()) { // Concurrent weak root processing @@ -2194,12 +2203,6 @@ void ShenandoahHeap::op_degenerated(ShenandoahDegenPoint point) { ShenandoahCodeRoots::disarm_nmethods(); } - { - ShenandoahTimingsTracker t(ShenandoahPhaseTimings::conc_weak_refs_work); - ShenandoahGCWorkerPhase worker_phase(ShenandoahPhaseTimings::conc_weak_refs_work); - ref_processor()->process_references(workers(), false /* concurrent */); - } - op_cleanup_early(); case _degenerated_evac: @@ -2485,6 +2488,7 @@ void ShenandoahHeap::stw_process_weak_roots(bool full_gc) { void ShenandoahHeap::parallel_cleaning(bool full_gc) { assert(SafepointSynchronize::is_at_safepoint(), "Must be at a safepoint"); assert(is_stw_gc_in_progress(), "Only for Degenerated and Full GC"); + stw_weak_refs(full_gc); stw_process_weak_roots(full_gc); stw_unload_classes(full_gc); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp index 90d809be6f4..5b163ecf84b 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp @@ -515,6 +515,7 @@ public: private: void stw_unload_classes(bool full_gc); void stw_process_weak_roots(bool full_gc); + void stw_weak_refs(bool full_gc); // Prepare concurrent root processing void prepare_concurrent_roots(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp b/src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp index 846b42c73ad..dfdf3fe698c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp @@ -249,7 +249,6 @@ void ShenandoahMarkCompact::phase1_mark_heap() { cm->mark_roots(ShenandoahPhaseTimings::full_gc_scan_roots); cm->finish_mark_from_roots(/* full_gc = */ true); heap->mark_complete_marking_context(); - rp->process_references(heap->workers(), false /* concurrent */); heap->parallel_cleaning(true /* full_gc */); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp b/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp index 08c1672c908..b39fb6b0d28 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp @@ -105,9 +105,11 @@ bool ShenandoahPhaseTimings::is_worker_phase(Phase phase) { case full_gc_adjust_roots: case degen_gc_scan_conc_roots: case degen_gc_update_roots: + case full_gc_weakrefs_process: case full_gc_scan_conc_roots: case full_gc_purge_class_unload: case full_gc_purge_weak_par: + case degen_gc_weakrefs_process: case degen_gc_purge_class_unload: case degen_gc_purge_weak_par: case heap_iteration_roots: diff --git a/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp b/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp index 07c3393f38f..bb12c58dd10 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp @@ -118,6 +118,8 @@ class outputStream; f(degen_gc, "Pause Degenerated GC (N)") \ f(degen_gc_scan_conc_roots, " Degen Mark Roots") \ SHENANDOAH_PAR_PHASE_DO(degen_gc_conc_mark_, " DM: ", f) \ + f(degen_gc_weakrefs, " Weak References") \ + f(degen_gc_weakrefs_process, " Process") \ f(degen_gc_purge, " System Purge") \ f(degen_gc_purge_class_unload, " Unload Classes") \ SHENANDOAH_PAR_PHASE_DO(degen_gc_purge_cu_par_, " DCU: ", f) \ From 4e8338eb13926775513c12492b8ccb8c9a84773e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Casta=C3=B1eda=20Lozano?= Date: Mon, 21 Dec 2020 13:04:24 +0000 Subject: [PATCH 2/7] 8255763: C2: OSR miscompilation caused by invalid memory instruction placement Disable GCM hoisting of memory-writing nodes for irreducible CFGs. This prevents GCM from wrongly "hoisting" stores into descendants of their original loop. Such an "inverted hoisting" can happen due to CFGLoop::compute_freq()'s inaccurate estimation of frequencies for irreducible CFGs. Extend CFG verification code by checking that memory-writing nodes are placed in either their original loop or an ancestor. Add tests for the reducible and irreducible cases. The former was already handled correctly before the change (the frequency estimation model prevents "inverted hoisting" for reducible CFGs), and is just added for coverage. This change addresses the specific miscompilation issue in a conservative way, for simplicity and safety. Future work includes investigating if only the illegal blocks can be discarded as candidates for GCM hoisting, and refining frequency estimation for irreducible CFGs. Reviewed-by: kvn, chagedorn --- src/hotspot/share/opto/block.cpp | 20 ++++ src/hotspot/share/opto/block.hpp | 4 +- src/hotspot/share/opto/gcm.cpp | 44 ++++++++- .../codegen/TestGCMStorePlacement.java | 97 +++++++++++++++++++ 4 files changed, 161 insertions(+), 4 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/codegen/TestGCMStorePlacement.java diff --git a/src/hotspot/share/opto/block.cpp b/src/hotspot/share/opto/block.cpp index 22e6d927c34..943a2fb30ca 100644 --- a/src/hotspot/share/opto/block.cpp +++ b/src/hotspot/share/opto/block.cpp @@ -1221,6 +1221,26 @@ void PhaseCFG::verify() const { if (j >= 1 && n->is_Mach() && n->as_Mach()->ideal_Opcode() == Op_CreateEx) { assert(j == 1 || block->get_node(j-1)->is_Phi(), "CreateEx must be first instruction in block"); } + // Verify that memory-writing nodes (such as stores and calls) are placed + // in their original loop L (given by the control input) or in an ancestor + // of L. This is guaranteed by the freq. estimation model for reducible + // CFGs, and by special handling in PhaseCFG::schedule_late() otherwise. + if (n->is_Mach() && n->bottom_type()->has_memory() && n->in(0) != NULL) { + Block* original_block = find_block_for_node(n->in(0)); + assert(original_block != NULL, "missing block for memory-writing node"); + CFGLoop* original_or_ancestor = original_block->_loop; + assert(block->_loop != NULL && original_or_ancestor != NULL, "no loop"); + bool found = false; + do { + if (block->_loop == original_or_ancestor) { + found = true; + break; + } + original_or_ancestor = original_or_ancestor->parent(); + } while (original_or_ancestor != NULL); + assert(found, "memory-writing node is not placed in its original loop " + "or an ancestor of it"); + } if (n->needs_anti_dependence_check()) { verify_anti_dependences(block, n); } diff --git a/src/hotspot/share/opto/block.hpp b/src/hotspot/share/opto/block.hpp index 775b3076db7..128be5e0381 100644 --- a/src/hotspot/share/opto/block.hpp +++ b/src/hotspot/share/opto/block.hpp @@ -501,8 +501,8 @@ class PhaseCFG : public Phase { CFGLoop* create_loop_tree(); bool is_dominator(Node* dom_node, Node* node); bool is_CFG(Node* n); - bool is_control_proj_or_safepoint(Node* n); - Block* find_block_for_node(Node* n); + bool is_control_proj_or_safepoint(Node* n) const; + Block* find_block_for_node(Node* n) const; bool is_dominating_control(Node* dom_ctrl, Node* n); #ifndef PRODUCT bool _trace_opto_pipelining; // tracing flag diff --git a/src/hotspot/share/opto/gcm.cpp b/src/hotspot/share/opto/gcm.cpp index 330809a4596..fe4437b97b9 100644 --- a/src/hotspot/share/opto/gcm.cpp +++ b/src/hotspot/share/opto/gcm.cpp @@ -152,14 +152,14 @@ bool PhaseCFG::is_CFG(Node* n) { return n->is_block_proj() || n->is_block_start() || is_control_proj_or_safepoint(n); } -bool PhaseCFG::is_control_proj_or_safepoint(Node* n) { +bool PhaseCFG::is_control_proj_or_safepoint(Node* n) const { bool result = (n->is_Mach() && n->as_Mach()->ideal_Opcode() == Op_SafePoint) || (n->is_Proj() && n->as_Proj()->bottom_type() == Type::CONTROL); assert(!result || (n->is_Mach() && n->as_Mach()->ideal_Opcode() == Op_SafePoint) || (n->is_Proj() && n->as_Proj()->_con == 0), "If control projection, it must be projection 0"); return result; } -Block* PhaseCFG::find_block_for_node(Node* n) { +Block* PhaseCFG::find_block_for_node(Node* n) const { if (n->is_block_start() || n->is_block_proj()) { return get_block_for_node(n); } else { @@ -1274,6 +1274,46 @@ void PhaseCFG::schedule_late(VectorSet &visited, Node_Stack &stack) { default: break; } + if (C->has_irreducible_loop() && self->bottom_type()->has_memory()) { + // If the CFG is irreducible, keep memory-writing nodes as close as + // possible to their original block (given by the control input). This + // prevents PhaseCFG::hoist_to_cheaper_block() from placing such nodes + // into descendants of their original loop, as in the following example: + // + // Original placement of store in B1 (loop L1): + // + // B1 (L1): + // m1 <- .. + // m2 <- store m1, .. + // B2 (L2): + // jump B2 + // B3 (L1): + // .. <- .. m2, .. + // + // Wrong "hoisting" of store to B2 (in loop L2, child of L1): + // + // B1 (L1): + // m1 <- .. + // B2 (L2): + // m2 <- store m1, .. + // # Wrong: m1 and m2 interfere at this point. + // jump B2 + // B3 (L1): + // .. <- .. m2, .. + // + // This "hoist inversion" can happen due to CFGLoop::compute_freq()'s + // inaccurate estimation of frequencies for irreducible CFGs, which can + // lead to for example assigning B1 and B3 a higher frequency than B2. +#ifndef PRODUCT + if (trace_opto_pipelining()) { + tty->print_cr("# Irreducible loops: schedule in earliest block B%d:", + early->_pre_order); + self->dump(); + } +#endif + schedule_node_into_block(self, early); + continue; + } } // Gather LCA of all uses diff --git a/test/hotspot/jtreg/compiler/codegen/TestGCMStorePlacement.java b/test/hotspot/jtreg/compiler/codegen/TestGCMStorePlacement.java new file mode 100644 index 00000000000..83600d0cb21 --- /dev/null +++ b/test/hotspot/jtreg/compiler/codegen/TestGCMStorePlacement.java @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2020, 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. + */ + +package compiler.codegen; + +import jdk.test.lib.Asserts; + +/** + * @test + * @bug 8255763 + * @summary Tests GCM's store placement for reducible and irreducible CFGs. + * @library /test/lib / + * @run main/othervm -Xbatch compiler.codegen.TestGCMStorePlacement reducible + * @run main/othervm -Xbatch compiler.codegen.TestGCMStorePlacement irreducible + */ + +public class TestGCMStorePlacement { + + static int counter; + + // Reducible case: counter++ should not be placed into the loop. + static void testReducible() { + counter++; + int acc = 0; + for (int i = 0; i < 50; i++) { + if (i % 2 == 0) { + acc += 1; + } + } + return; + } + + // Irreducible case (due to OSR compilation): counter++ should not be placed + // outside its switch case block. + static void testIrreducible() { + for (int i = 0; i < 30; i++) { + switch (i % 3) { + case 0: + for (int j = 0; j < 50; j++) { + // OSR enters here. + for (int k = 0; k < 7000; k++) {} + if (i % 2 == 0) { + break; + } + } + counter++; + break; + case 1: + break; + case 2: + break; + } + } + return; + } + + public static void main(String[] args) { + switch (args[0]) { + case "reducible": + // Cause a regular C2 compilation of testReducible. + for (int i = 0; i < 100_000; i++) { + counter = 0; + testReducible(); + Asserts.assertEQ(counter, 1); + } + break; + case "irreducible": + // Cause an OSR C2 compilation of testIrreducible. + counter = 0; + testIrreducible(); + Asserts.assertEQ(counter, 10); + break; + default: + System.out.println("invalid mode"); + } + } +} From 8da7c58016342e88e9f32ee23550a91b773001ab Mon Sep 17 00:00:00 2001 From: Jonathan Gibbons Date: Mon, 21 Dec 2020 17:04:37 +0000 Subject: [PATCH 3/7] 8258443: doclint should be service-loaded with system class loader Reviewed-by: alanb --- .../share/classes/com/sun/tools/doclint/DocLint.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jdk.compiler/share/classes/com/sun/tools/doclint/DocLint.java b/src/jdk.compiler/share/classes/com/sun/tools/doclint/DocLint.java index f14aa349d10..f4e6e241bbe 100644 --- a/src/jdk.compiler/share/classes/com/sun/tools/doclint/DocLint.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/doclint/DocLint.java @@ -50,7 +50,7 @@ public abstract class DocLint implements Plugin { public static synchronized DocLint newDocLint() { if (docLintProvider == null) { - docLintProvider = ServiceLoader.load(DocLint.class).stream() + docLintProvider = ServiceLoader.load(DocLint.class, ClassLoader.getSystemClassLoader()).stream() .filter(p_ -> p_.get().getName().equals("doclint")) .findFirst() .orElse(new ServiceLoader.Provider<>() { From 772addfd24615078c029c1f01f374b5ef58a3cb6 Mon Sep 17 00:00:00 2001 From: Vladimir Ivanov Date: Tue, 22 Dec 2020 12:12:21 +0000 Subject: [PATCH 4/7] 8258790: C2: Crash on empty macro node list Reviewed-by: kvn, chagedorn --- src/hotspot/share/opto/macro.cpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/hotspot/share/opto/macro.cpp b/src/hotspot/share/opto/macro.cpp index 3b335471974..cc6b6658ac7 100644 --- a/src/hotspot/share/opto/macro.cpp +++ b/src/hotspot/share/opto/macro.cpp @@ -2563,11 +2563,8 @@ void PhaseMacroExpand::eliminate_macro_nodes() { bool progress = true; while (progress) { progress = false; - for (int i = C->macro_count(); i > 0; i--) { - if (i > C->macro_count()) { - i = C->macro_count(); // more than 1 element can be eliminated at once - } - Node* n = C->macro_node(i-1); + for (int i = C->macro_count(); i > 0; i = MIN2(i - 1, C->macro_count())) { // more than 1 element can be eliminated at once + Node* n = C->macro_node(i - 1); bool success = false; DEBUG_ONLY(int old_macro_count = C->macro_count();) if (n->is_AbstractLock()) { @@ -2582,11 +2579,8 @@ void PhaseMacroExpand::eliminate_macro_nodes() { progress = true; while (progress) { progress = false; - for (int i = C->macro_count(); i > 0; i--) { - if (i > C->macro_count()) { - i = C->macro_count(); // more than 1 element can be eliminated at once - } - Node* n = C->macro_node(i-1); + for (int i = C->macro_count(); i > 0; i = MIN2(i - 1, C->macro_count())) { // more than 1 element can be eliminated at once + Node* n = C->macro_node(i - 1); bool success = false; DEBUG_ONLY(int old_macro_count = C->macro_count();) switch (n->class_id()) { From 88dd6a94347ed3e5d48717003b28052d81f99f0c Mon Sep 17 00:00:00 2001 From: "Daniel D. Daugherty" Date: Tue, 22 Dec 2020 13:43:17 +0000 Subject: [PATCH 5/7] 8258802: ProblemList TestJstatdDefaults.java, TestJstatdRmiPort.java, and TestJstatdServer.java Reviewed-by: amenkov, cjplummer --- test/jdk/ProblemList.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt index 7423d148da9..2644c6a0618 100644 --- a/test/jdk/ProblemList.txt +++ b/test/jdk/ProblemList.txt @@ -824,6 +824,10 @@ com/sun/jdi/InvokeHangTest.java 8218463 linux-al sun/tools/jhsdb/BasicLauncherTest.java 8211767 linux-ppc64,linux-ppc64le +sun/tools/jstatd/TestJstatdDefaults.java 8081569,8226420 windows-all +sun/tools/jstatd/TestJstatdRmiPort.java 8226420,8251259 windows-all +sun/tools/jstatd/TestJstatdServer.java 8081569,8226420 windows-all + ############################################################################ From eabc9030ab86b19d79e3ca559f3312445de92f44 Mon Sep 17 00:00:00 2001 From: "Daniel D. Daugherty" Date: Tue, 22 Dec 2020 17:15:34 +0000 Subject: [PATCH 6/7] 8258827: ProblemList Naming/DefaultRegistryPort.java and Naming/legalRegistryNames/LegalRegistryNames.java on Windows Reviewed-by: rriggs, msheppar, prr --- test/jdk/ProblemList.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt index 2644c6a0618..02ac31d79ff 100644 --- a/test/jdk/ProblemList.txt +++ b/test/jdk/ProblemList.txt @@ -646,6 +646,9 @@ java/rmi/activation/rmidViaInheritedChannel/InheritedChannelNotServerSocket.java java/rmi/registry/readTest/CodebaseTest.java 8173324 windows-all +java/rmi/Naming/DefaultRegistryPort.java 8005619 windows-all +java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java 8005619 windows-all + ############################################################################ # jdk_sctp From 61e5e393a7d3d86fb6714849af10a0580a2610eb Mon Sep 17 00:00:00 2001 From: "Daniel D. Daugherty" Date: Tue, 22 Dec 2020 18:59:36 +0000 Subject: [PATCH 7/7] 8258832: ProblemList com/sun/jdi/AfterThreadDeathTest.java on Linux-X64 Reviewed-by: ccheung, amenkov --- test/jdk/ProblemList.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt index 02ac31d79ff..ed22bf12351 100644 --- a/test/jdk/ProblemList.txt +++ b/test/jdk/ProblemList.txt @@ -809,6 +809,8 @@ com/sun/jdi/NashornPopFrameTest.java 8225620 generic- com/sun/jdi/InvokeHangTest.java 8218463 linux-all +com/sun/jdi/AfterThreadDeathTest.java 8232839 linux-x64 + ############################################################################ # jdk_time