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

[Jon Masamitsu]

Sangheon,

Thanks.

Jon

On 7/29/2016 8:46 AM, sangheon wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>