Request for Review (xs) - 8159073: Error handling incomplete when creating GC threads lazily

[sangheon]

Hi Jon,

On 07/29/2016 07:48 AM, Jon Masamitsu wrote:
> Sangheon,
>
> Thank you for looking at this review.
>
> On 7/29/2016 12:42 AM, sangheon wrote:
>> Hi Jon,
>>
>> Looking at the ver.03+delta_03_04, I have a question.
>> src/share/vm/gc/parallel/psScavenge.cpp.frames.html
>> line 398, previously we checked active_workers but you are suggesting 
>> to check total workers.
>> Is there any reason on this? Looking at the new comment, just adding 
>> the comment without code change seems correct.
>
> I believe the change is necessary.  In PSPromotionManager there is a 
> field
> _totally_drain that is set in the PSPromotionManager constructor to
>
> 196   _totally_drain = (ParallelGCThreads == 1) || 
> (GCDrainStackTargetSize == 0)
>
> The promotion managers live for the life of the JVM so are not 
> constructed
> for each collection.  If _totally_drain is not set, then the promotion 
> manager
> expects there to be stealing tasks and does not fully drain the 
> marking stacks
> (expecting the stealing tasks to finish draining the marking stacks).  
> active_workers
> might be set to 1 because of ergonomics but a stealing task is still 
> needed
> (if ParallelGCThreads is > 1).  So the test is changed to add a 
> stealing task
> if the total workers is greater than 1 (meaning even if active_workers 
> == 1,
> a stealing task is still needed).
>
> Thomas brought up the point of this special case code and it should 
> probably
> be removed.  There is another special case which similarly does something
> different if there is only 1 GC worker to perform a task.  That 
> special case
> uses the VM thread to execute the task.  Both these special cases 
> probably
> should be removed (any performance benefit is not worth the special
> case for 1 GC worker).
Okay, thanks for the explanation.
I thought I followed previous discussions between you and Thomas, but I 
mis-understood this part.

>
>>
>> Please update the copyright in TestGCOld.java before pushing this if 
>> you care.
>
> Done.
Thanks.

>
> Diff on TestGCOld.java
>
>> --- a/test/gc/stress/TestGCOld.java
>> +++ b/test/gc/stress/TestGCOld.java
>> @@ -1,5 +1,5 @@
>>  /*
>> -* Copyright (c) 2015, Oracle and/or its affiliates. All rights 
>> reserved.
>> +* Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights 
>> reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>  *
>>  * This code is free software; you can redistribute it and/or modify it
>
> Latest delta webrev.
>
> http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_04_05/
Looks good.

Thanks,
Sangheon


>
> Thanks again.
>
> Jon
>>
>> Thanks,
>> Sangheon
>>
>>
>> On 07/28/2016 02:54 PM, Jon Masamitsu wrote:
>>> I still need one more.
>>>
>>> http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_03_04/
>>>
>>> Thanks.
>>>
>>> Jon
>>>
>>> On 07/26/2016 08:36 AM, Jon Masamitsu wrote:
>>>> Thomas,
>>>>
>>>> Thanks.
>>>>
>>>> Jon
>>>>
>>>> On 7/26/2016 1:36 AM, Thomas Schatzl wrote:
>>>>> Hi Jon,
>>>>>
>>>>> On Mon, 2016-07-25 at 15:43 -0700, Jon Masamitsu wrote:
>>>>>>
>>>>>> On 07/21/2016 06:57 AM, Thomas Schatzl wrote:
>>>>>>> Hi Jon,
>>>>>>>
>>>>>>>
>>>>> [...]
>>>>>>>> Delta: http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_02_
>>>>>>>> 03/
>>>>>>>> Full: http://cr.openjdk.java.net/~jmasa/8159073/webrev.03/
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>    looks good, ship it :)
>>>>>>   When I ran tests which injected  thread creation failure, this
>>>>>> guarantee failed.
>>>>>>
>>>>>> diff --git a/src/share/vm/gc/shared/workgroup.cpp
>>>>>> b/src/share/vm/gc/shared/workgroup.cpp
>>>>>> --- a/src/share/vm/gc/shared/workgroup.cpp
>>>>>> +++ b/src/share/vm/gc/shared/workgroup.cpp
>>>>>> @@ -276,7 +276,6 @@
>>>>>>     guarantee(num_workers > 0, "Trying to execute task %s with zero
>>>>>> workers", task->name());
>>>>>>     uint old_num_workers = _active_workers;
>>>>>>     update_active_workers(num_workers);
>>>>>> -  guarantee(_active_workers == num_workers, "active workers %u
>>>>>> num_workers %u", _active_workers, num_workers);
>>>>>>     _dispatcher->coordinator_execute_on_workers(task, num_workers);
>>>>>>     update_active_workers(old_num_workers);
>>>>>>   }
>>>>>>
>>>>>> "num_workers" was 2 and the update_active_workers() failed to create
>>>>>> an
>>>>>> additional thread so "_active_workers" remained at 1.  I deleted the
>>>>>> guarantee.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_03_04/
>>>>>    looks good.
>>>>>
>>>>> Thomas
>>>>>
>>>>
>>>
>>
>