Request for review JDK-8140588 - Internal Error: gc/g1/ptrQueue.hpp:126 assert(_index == _sz) failed: invariant: queues are empty when activated
Per Liden
per.liden at oracle.com
Tue Jan 10 10:04:00 UTC 2017
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
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>
>
More information about the ppc-aix-port-dev
mailing list