RFR(XXS): 8153176: ParOldGC's ParallelTaskTerminator peeks wrong TaskQueueSet

[Mikael Gerdin]

Hi Richard,

On 2016-04-01 14:53, Reingruber, Richard wrote:
> Hi Mikael,
>
>    > > Hi Mikael,
>    > >
>    > >    Mikael> As I mentioned in the bug I think the same problem affects parallel
>    > >    Mikael> reference processing as well, see RefProcTaskExecutor::execute in
>    > >    Mikael> pcTasks.cpp.
>    > >
>    > > Oh cool you saw this! Should I change this, too?
>    >
>    > Yes, it's just as incorrect as the one you originally found.
>
> Ok, I have prepared a second webrev that fixes RefProcTaskExecutor::execute(), too.
>
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8153176_paroldgc_wrong_taskqueueset_in_marking/webrev_02/
>
> Could you please sponsor the change, when it's reviewed?

Yep. I'll take care of it.

/Mikael

>
> Thanks,
> Richard.
>
> -----Original Message-----
> From: Mikael Gerdin [mailto:mikael.gerdin at oracle.com]
> Sent: Donnerstag, 31. März 2016 17:35
> To: Reingruber, Richard <richard.reingruber at sap.com>; hotspot-gc-dev at openjdk.java.net
> Subject: Re: RFR(XXS): 8153176: ParOldGC's ParallelTaskTerminator peeks wrong TaskQueueSet
>
> Hi Richard,
>
> On 2016-03-31 17:31, Reingruber, Richard wrote:
>> Hi Mikael,
>>
>>     Mikael> As I mentioned in the bug I think the same problem affects parallel
>>     Mikael> reference processing as well, see RefProcTaskExecutor::execute in
>>     Mikael> pcTasks.cpp.
>>
>> Oh cool you saw this! Should I change this, too?
>
> Yes, it's just as incorrect as the one you originally found.
>
>>
>>     Mikael> Another fun thing is that if there are only object arrays on any
>>     Mikael> thread's work stealing queue then work stealing also breaks down,
>>     Mikael> because the terminator doesn't check the _objarray_queues.
>>
>> Yes, I noticed this as well. If another thread is processing a large object
>> array, then the elements will show up on its marking stack. This will let the
>> thread spinning in offer_termination() return and continue in
>> StealMarkingTask::do_it(), where it will try to steal from the
>> _objarray_queues. I thought this might be sufficient.
>
> Ah, right. That should save us then.
>
> /Mikael
>
>>
>> Cheers, Richard.
>>
>> -----Original Message-----
>> From: Mikael Gerdin [mailto:mikael.gerdin at oracle.com]
>> Sent: Donnerstag, 31. März 2016 16:17
>> To: Reingruber, Richard <richard.reingruber at sap.com>; hotspot-gc-dev at openjdk.java.net
>> Subject: Re: RFR(XXS): 8153176: ParOldGC's ParallelTaskTerminator peeks wrong TaskQueueSet
>>
>> Hi Richard,
>>
>> On 2016-03-31 15:55, Reingruber, Richard wrote:
>>> Hi,
>>>
>>> I would like to contribute a fix for a bug in parallel old gc. The bug can cause long gc pauses, because work stealing while marking does not work as ParallelTaskTerminator peeks the wrong task queue set.
>>>
>>> A microbenchmark that produces an object graph with large linear parts consistently shows 3x shorter gc pauses with the fix (see bug report). In a similar synthetic test the bug sporadically caused pauses 10x longer than average.
>>>
>>> Webrev: http://www.sapjvm.com/rr/webrevs/8153176_paroldgc_wrong_taskqueueset_in_marking/webrev_01/
>>
>> As I mentioned in the bug I think the same problem affects parallel
>> reference processing as well, see RefProcTaskExecutor::execute in
>> pcTasks.cpp.
>>
>> Another fun thing is that if there are only object arrays on any
>> thread's work stealing queue then work stealing also breaks down,
>> because the terminator doesn't check the _objarray_queues.
>>
>> Perhaps as a future change ParallelTaskTerminator could be decoupled
>> from TaskQueueSetSuper and instead using either template based
>> polymorphism or virtual calls to do the "peek in queue set" callback.
>>
>> /Mikael
>>
>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8153176
>>>
>>> The change needs to be sponsored as well, please.
>>>
>>> Thanks, Richard.
>>>
>>> --
>>> Richard Reingruber | SAP JVM | CORE PLATFORM,  SAP SE
>>> Pflichtangaben/Mandatory Disclosure Statements: http://www.sap.com/company/legal/impressum.epx
>>>