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

[Jon Masamitsu]

On 07/21/2016 06:57 AM, Thomas Schatzl wrote:
> Hi Jon,
>
> On Thu, 2016-07-21 at 06:48 -0700, Jon Masamitsu wrote:
>> On 7/21/2016 1:20 AM, Thomas Schatzl wrote:
>>> Hi Jon,
>>>
>>> On Wed, 2016-07-20 at 20:43 -0700, Jon Masamitsu wrote:
>>>> It's ready again.
>>>>
>>>> Delta: http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_01_
>>>> 02/
>>>> Full: http://cr.openjdk.java.net/~jmasa/8159073/webrev.02/
>>>>
>>>     this looks good, except for the two log messages. They seem to
>>> be
>>> superfluous because first the callers of run_task(, uint) already
>>> print
>>> mostly the same message, and the given code guarantee's that
>>> whatever
>>> is printed anyway. Further, a single invocation of run_task will
>>> now
>>> trigger three messages basically telling the same.
>>>
>>> So I would prefer if these messages were either moved to trace
>>> level,
>>> or just removed.
>>>
>>> Thanks a lot for your patience with me, :)
>>>     Thomas
>> I deleted the log messages.
>>
>> --- a/src/share/vm/gc/shared/workgroup.cpp
>> +++ b/src/share/vm/gc/shared/workgroup.cpp
>> @@ -272,11 +272,9 @@
>>                task->name(), num_workers, total_workers());
>>      guarantee(num_workers > 0, "Trying to execute task %s with zero
>> workers", task->name());
>>      uint old_num_workers = _active_workers;
>> -  log_debug(gc)("run_task: updating active workers for %s from %u
>> to
>> %u", task->name(), old_num_workers, num_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);
>> -  log_debug(gc)("run_task: restoring active workers from %u to %u",
>> num_workers, old_num_workers);
>>      update_active_workers(old_num_workers);
>>    }
>>
>> 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/

Jon

>
> Thanks,
>    Thomas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160725/4b1ca2d0/attachment.htm>