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

[Alexander Harlap]

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