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:per.liden at oracle.com] 
Sent: Montag, 9. Januar 2017 10:23
To: Alexander Harlap <alexander.harlap at oracle.com>; Kim Barrett <kim.barrett at oracle.com>; Doerr, Martin <martin.doerr at sap.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

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
>>>> <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.
>>
>> 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
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>