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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap

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.


Alex

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Dec 24, 2016, at 10:04 AM, Alexander Harlap <[hidden email]> 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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Per Liden
Hi,

On 2017-01-05 00:36, Kim Barrett wrote:

>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap <[hidden email]> 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.

Agree. If there's no measurable performance gain by having two stubs
(given the other things we do in this path I'd be surprised if there
were) I'd vote for keeping it simple here and have one stub. That would
also "fix" Kim's last comment below.

cheers,
Per

>
> ------------------------------------------------------------------------------
> 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
>
> ------------------------------------------------------------------------------
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Doerr, Martin
In reply to this post by Kim Barrett
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:[hidden email]] On Behalf Of Kim Barrett
Sent: Donnerstag, 5. Januar 2017 00:36
To: Alexander Harlap <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]
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 <[hidden email]> 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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: 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:[hidden email]] On Behalf Of Doerr, Martin
Sent: Donnerstag, 5. Januar 2017 14:40
To: Kim Barrett <[hidden email]>; Alexander Harlap <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]
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:[hidden email]] On Behalf Of Kim Barrett
Sent: Donnerstag, 5. Januar 2017 00:36
To: Alexander Harlap <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]
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 <[hidden email]> 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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
In reply to this post by Per Liden
Change at http://cr.openjdk.java.net/~aharlap/8140588/webrev.01/

accommodates comments from Per, Kim and Martin

Yes, there is no measurable performance difference.

So make change simpler.

Alex

On 1/5/2017 7:41 AM, Per Liden wrote:

> Hi,
>
> On 2017-01-05 00:36, Kim Barrett wrote:
>>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap
>>> <[hidden email]> 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.
>
> Agree. If there's no measurable performance gain by having two stubs
> (given the other things we do in this path I'd be surprised if there
> were) I'd vote for keeping it simple here and have one stub. That
> would also "fix" Kim's last comment below.
>
> cheers,
> Per
>
>>
>> ------------------------------------------------------------------------------
>>
>> 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
>>
>> ------------------------------------------------------------------------------
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Per Liden
Hi Alex,

On 2017-01-06 17:35, Alexander Harlap wrote:
> Change at http://cr.openjdk.java.net/~aharlap/8140588/webrev.01/

The x86 and Sparc parts looks good, I'll let others comments on the
other ports. However, a few comments:


c1_Runtime1_ppc.cpp
-------------------
  764
guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
"Assumption");

Please change this to be an assert() instead of a guarantee().


c1_Runtime1_s390.cpp
--------------------
  806
guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
"Assumption");

Same here.

c1_Runtime1_sparc.cpp
---------------------
  873
guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
  874                    "Assumption");

And same here.


cheers,
/Per

>
> accommodates comments from Per, Kim and Martin
>
> Yes, there is no measurable performance difference.
>
> So make change simpler.
>
> Alex
>
> On 1/5/2017 7:41 AM, Per Liden wrote:
>> Hi,
>>
>> On 2017-01-05 00:36, Kim Barrett wrote:
>>>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap
>>>> <[hidden email]> 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.
>>
>> Agree. If there's no measurable performance gain by having two stubs
>> (given the other things we do in this path I'd be surprised if there
>> were) I'd vote for keeping it simple here and have one stub. That
>> would also "fix" Kim's last comment below.
>>
>> cheers,
>> Per
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> 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
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Doerr, Martin
Hi Alex,

the PPC64 and s390 parts look good, too.

Thanks,
Martin

-----Original Message-----
From: Per Liden [mailto:[hidden email]]
Sent: Montag, 9. Januar 2017 10:23
To: Alexander Harlap <[hidden email]>; Kim Barrett <[hidden email]>; Doerr, Martin <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]
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 Alex,

On 2017-01-06 17:35, Alexander Harlap wrote:
> Change at http://cr.openjdk.java.net/~aharlap/8140588/webrev.01/

The x86 and Sparc parts looks good, I'll let others comments on the other ports. However, a few comments:


c1_Runtime1_ppc.cpp
-------------------
  764
guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
"Assumption");

Please change this to be an assert() instead of a guarantee().


c1_Runtime1_s390.cpp
--------------------
  806
guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
"Assumption");

Same here.

c1_Runtime1_sparc.cpp
---------------------
  873
guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
  874                    "Assumption");

And same here.


cheers,
/Per

>
> accommodates comments from Per, Kim and Martin
>
> Yes, there is no measurable performance difference.
>
> So make change simpler.
>
> Alex
>
> On 1/5/2017 7:41 AM, Per Liden wrote:
>> Hi,
>>
>> On 2017-01-05 00:36, Kim Barrett wrote:
>>>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap
>>>> <[hidden email]> 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.
>>
>> Agree. If there's no measurable performance gain by having two stubs
>> (given the other things we do in this path I'd be surprised if there
>> were) I'd vote for keeping it simple here and have one stub. That
>> would also "fix" Kim's last comment below.
>>
>> cheers,
>> Per
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> 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
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
In reply to this post by Per Liden
Here is change with consistent use of assert:

http://cr.openjdk.java.net/~aharlap/8140588/webrev.02/


Alex


On 1/9/2017 4:23 AM, Per Liden wrote:

> Hi Alex,
>
> On 2017-01-06 17:35, Alexander Harlap wrote:
>> Change at http://cr.openjdk.java.net/~aharlap/8140588/webrev.01/
>
> The x86 and Sparc parts looks good, I'll let others comments on the
> other ports. However, a few comments:
>
>
> c1_Runtime1_ppc.cpp
> -------------------
>  764 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
> "Assumption");
>
> Please change this to be an assert() instead of a guarantee().
>
>
> c1_Runtime1_s390.cpp
> --------------------
>  806 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
> "Assumption");
>
> Same here.
>
> c1_Runtime1_sparc.cpp
> ---------------------
>  873 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>  874                    "Assumption");
>
> And same here.
>
>
> cheers,
> /Per
>
>>
>> accommodates comments from Per, Kim and Martin
>>
>> Yes, there is no measurable performance difference.
>>
>> So make change simpler.
>>
>> Alex
>>
>> On 1/5/2017 7:41 AM, Per Liden wrote:
>>> Hi,
>>>
>>> On 2017-01-05 00:36, Kim Barrett wrote:
>>>>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap
>>>>> <[hidden email]> 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.
>>>
>>> Agree. If there's no measurable performance gain by having two stubs
>>> (given the other things we do in this path I'd be surprised if there
>>> were) I'd vote for keeping it simple here and have one stub. That
>>> would also "fix" Kim's last comment below.
>>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> 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
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Per Liden
Hi Alex,

Looks good, just two nitpicks for the sake of uniformity across the
platforms:

c1_Runtime1_sparc.cpp
---------------------
  873           assert(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
  874                    "Assumption");

Indentation looks off here, but please make this one line instead, like
the other platforms.


c1_Runtime1_arm.cpp
-------------------
  565         assert(in_bytes(SATBMarkQueue::byte_width_of_active()) ==
1, "adjust this code");

Please say "Assumption", like the other platforms.


If you fix these I don't need to see a new webrev.

cheers,
Per


On 2017-01-09 20:31, Alexander Harlap wrote:

> Here is change with consistent use of assert:
>
> http://cr.openjdk.java.net/~aharlap/8140588/webrev.02/
>
>
> Alex
>
>
> On 1/9/2017 4:23 AM, Per Liden wrote:
>> Hi Alex,
>>
>> On 2017-01-06 17:35, Alexander Harlap wrote:
>>> Change at http://cr.openjdk.java.net/~aharlap/8140588/webrev.01/
>>
>> The x86 and Sparc parts looks good, I'll let others comments on the
>> other ports. However, a few comments:
>>
>>
>> c1_Runtime1_ppc.cpp
>> -------------------
>>  764 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>> "Assumption");
>>
>> Please change this to be an assert() instead of a guarantee().
>>
>>
>> c1_Runtime1_s390.cpp
>> --------------------
>>  806 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>> "Assumption");
>>
>> Same here.
>>
>> c1_Runtime1_sparc.cpp
>> ---------------------
>>  873 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>>  874                    "Assumption");
>>
>> And same here.
>>
>>
>> cheers,
>> /Per
>>
>>>
>>> accommodates comments from Per, Kim and Martin
>>>
>>> Yes, there is no measurable performance difference.
>>>
>>> So make change simpler.
>>>
>>> Alex
>>>
>>> On 1/5/2017 7:41 AM, Per Liden wrote:
>>>> Hi,
>>>>
>>>> On 2017-01-05 00:36, Kim Barrett wrote:
>>>>>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap
>>>>>> <[hidden email]> 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.
>>>>
>>>> Agree. If there's no measurable performance gain by having two stubs
>>>> (given the other things we do in this path I'd be surprised if there
>>>> were) I'd vote for keeping it simple here and have one stub. That
>>>> would also "fix" Kim's last comment below.
>>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>> 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
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
I accommodated latest comment from Per.

Should I wait for more reviews?

Alex


On 1/10/2017 5:04 AM, Per Liden wrote:

> Hi Alex,
>
> Looks good, just two nitpicks for the sake of uniformity across the
> platforms:
>
> c1_Runtime1_sparc.cpp
> ---------------------
>  873 assert(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>  874                    "Assumption");
>
> Indentation looks off here, but please make this one line instead,
> like the other platforms.
>
>
> c1_Runtime1_arm.cpp
> -------------------
>  565 assert(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
> "adjust this code");
>
> Please say "Assumption", like the other platforms.
>
>
> If you fix these I don't need to see a new webrev.
>
> cheers,
> Per
>
>
> On 2017-01-09 20:31, Alexander Harlap wrote:
>> Here is change with consistent use of assert:
>>
>> http://cr.openjdk.java.net/~aharlap/8140588/webrev.02/
>>
>>
>> Alex
>>
>>
>> On 1/9/2017 4:23 AM, Per Liden wrote:
>>> Hi Alex,
>>>
>>> On 2017-01-06 17:35, Alexander Harlap wrote:
>>>> Change at http://cr.openjdk.java.net/~aharlap/8140588/webrev.01/
>>>
>>> The x86 and Sparc parts looks good, I'll let others comments on the
>>> other ports. However, a few comments:
>>>
>>>
>>> c1_Runtime1_ppc.cpp
>>> -------------------
>>>  764 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>>> "Assumption");
>>>
>>> Please change this to be an assert() instead of a guarantee().
>>>
>>>
>>> c1_Runtime1_s390.cpp
>>> --------------------
>>>  806 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>>> "Assumption");
>>>
>>> Same here.
>>>
>>> c1_Runtime1_sparc.cpp
>>> ---------------------
>>>  873 guarantee(in_bytes(SATBMarkQueue::byte_width_of_active()) == 1,
>>>  874                    "Assumption");
>>>
>>> And same here.
>>>
>>>
>>> cheers,
>>> /Per
>>>
>>>>
>>>> accommodates comments from Per, Kim and Martin
>>>>
>>>> Yes, there is no measurable performance difference.
>>>>
>>>> So make change simpler.
>>>>
>>>> Alex
>>>>
>>>> On 1/5/2017 7:41 AM, Per Liden wrote:
>>>>> Hi,
>>>>>
>>>>> On 2017-01-05 00:36, Kim Barrett wrote:
>>>>>>> On Dec 24, 2016, at 10:04 AM, Alexander Harlap
>>>>>>> <[hidden email]> 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.
>>>>>
>>>>> Agree. If there's no measurable performance gain by having two stubs
>>>>> (given the other things we do in this path I'd be surprised if there
>>>>> were) I'd vote for keeping it simple here and have one stub. That
>>>>> would also "fix" Kim's last comment below.
>>>>>
>>>>> cheers,
>>>>> Per
>>>>>
>>>>>>
>>>>>> ------------------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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
>>>>>>
>>>>>> ------------------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
In reply to this post by Alexander Harlap
> On Jan 9, 2017, at 2:31 PM, Alexander Harlap <[hidden email]> wrote:
>
> Here is change with consistent use of assert:
>
> http://cr.openjdk.java.net/~aharlap/8140588/webrev.02/


Looks good.

Loading...