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

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