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

[Jon Masamitsu]

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

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

Done.

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/

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