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

[Alexander Harlap]

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