Request for review JDK-8140588 - Internal Error: gc/g1/ptrQueue.hpp:126 assert(_index == _sz) failed: invariant: queues are empty when activated

[Doerr, Martin]

Sorry, pasting patches into emails doesn't work as expected.
I've uploaded the patch here:
http://cr.openjdk.java.net/~mdoerr/8140588_webrev00_addon.patch

Thanks for porting the changes to PPC64/s390.

Best regards,
Martin


-----Original Message-----
From: hotspot-gc-dev [mailto:hotspot-gc-dev-bounces at openjdk.java.net] On Behalf Of Doerr, Martin
Sent: Donnerstag, 5. Januar 2017 14:40
To: Kim Barrett <kim.barrett at oracle.com>; Alexander Harlap <alexander.harlap at oracle.com>
Cc: hotspot-gc-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net; aarch64-port-dev at openjdk.java.net
Subject: RE: Request for review JDK-8140588 - Internal Error: gc/g1/ptrQueue.hpp:126 assert(_index == _sz) failed: invariant: queues are empty when activated

Hi,

it'd be nice if you could apply the following patch to webrev.00 to fix PPC64/s390 build.

Thanks,
Martin

******** START
--- a/src/cpu/ppc/vm/c1_CodeStubs_ppc.cpp       Thu Jan 05 14:08:29 2017 +0100
+++ b/src/cpu/ppc/vm/c1_CodeStubs_ppc.cpp       Thu Jan 05 14:35:32 2017 +0100
@@ -492,7 +492,7 @@
   __ cmpdi(CCR0, pre_val_reg, 0);
   __ bc_far_optimized(Assembler::bcondCRbiIs1, __ bi0(CCR0, Assembler::equal), _continuation);

-  Runtime1::StubID id = patch_code() = lir_patch_none ? Runtime1::g1_pre_barrier_slow_id : Runtime1::g1_pre_barrier_slow_with_recheck_id;
+  Runtime1::StubID id = patch_code() == lir_patch_none ? 
+ Runtime1::g1_pre_barrier_slow_id : 
+ Runtime1::g1_pre_barrier_slow_with_recheck_id;
   address stub = Runtime1::entry_for(id);
   //__ load_const_optimized(R0, stub);
   __ add_const_optimized(R0, R29_TOC, MacroAssembler::offset_to_global_toc(stub));
diff -r 0f48eb902cfc src/cpu/ppc/vm/c1_Runtime1_ppc.cpp
--- a/src/cpu/ppc/vm/c1_Runtime1_ppc.cpp        Thu Jan 05 14:08:29 2017 +0100
+++ b/src/cpu/ppc/vm/c1_Runtime1_ppc.cpp        Thu Jan 05 14:35:32 2017 +0100
@@ -746,8 +746,8 @@
         Register tmp  = R14;
         Register tmp2 = R15;

-        Label refill, restart, marking_not_active;;
-
+        Label refill, restart, marking_not_active;
+
         int satb_q_active_byte_offset =
           in_bytes(JavaThread::satb_mark_queue_offset() +
                    SATBMarkQueue::byte_offset_of_active());
diff -r 0f48eb902cfc src/cpu/s390/vm/c1_Runtime1_s390.cpp
--- a/src/cpu/s390/vm/c1_Runtime1_s390.cpp      Thu Jan 05 14:08:29 2017 +0100
+++ b/src/cpu/s390/vm/c1_Runtime1_s390.cpp      Thu Jan 05 14:35:32 2017 +0100
@@ -810,7 +810,7 @@
             __ load_and_test_int(tmp, Address(Z_thread, satb_q_active_byte_offset));
           } else {
             guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1, "Assumption");
-            __ load_and_test_byte(tmp, Address(Z_thread, satb_q_active_byte_offset`));
+            __ load_and_test_byte(tmp, Address(Z_thread, 
+ satb_q_active_byte_offset));
           }
           __ z_bre(marking_not_active); // Activity indicator is zero, so there is no marking going on currently.
         }
******** END

-----Original Message-----
From: hotspot-gc-dev [mailto:hotspot-gc-dev-bounces at openjdk.java.net] On Behalf Of Kim Barrett
Sent: Donnerstag, 5. Januar 2017 00:36
To: Alexander Harlap <alexander.harlap at oracle.com>
Cc: ppc-aix-port-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net; aarch64-port-dev at openjdk.java.net
Subject: Re: Request for review JDK-8140588 - Internal Error: gc/g1/ptrQueue.hpp:126 assert(_index == _sz) failed: invariant: queues are empty when activated

> On Dec 24, 2016, at 10:04 AM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> 
> Please review change for JDK-8140588 - Internal Error: 
> gc/g1/ptrQueue.hpp:126 assert(_index == _sz) failed: invariant: queues 
> are empty when activated
> 
> Change is located at
> http://cr.openjdk.java.net/~aharlap/8140588/webrev.00/
> 
> It implements Per Liden fix for rechecking value of marking in progress value inside C1 g1_pre_barrier code.
> 
> Also here is optimization described by Thomas Schatzl - do recheck only if patching involved.

I wonder whether it is worth the effort of having distinct stubs for the two cases, or just unconditionally perform the recheck in the existing stub.  Thomas said he found no performance issue with the simpler version.

------------------------------------------------------------------------------
Throughout, copyright dates need updating.

------------------------------------------------------------------------------
cpu/aarch64/vm/c1_CodeStubs_aarch64.cpp
 373   Runtime1::StubID id = patch_code() == lir_patch_none ? Runtime1::g1_pre_barrier_slow_id : Runtime1::g1_pre_barrier_slow_with_recheck_id;

Assuming we stay with the pair of stubs, this ought to be packaged up as a helper function.

Other occurrences:
src/cpu/arm/vm/c1_CodeStubs_arm.cpp
src/cpu/ppc/vm/c1_CodeStubs_ppc.cpp
src/cpu/s390/vm/c1_CodeStubs_s390.cpp
src/cpu/sparc/vm/c1_CodeStubs_sparc.cpp
src/cpu/x86/vm/c1_CodeStubs_x86.cpp

------------------------------------------------------------------------------