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

[Jon Masamitsu]

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/

Jon

On 7/20/2016 12:52 PM, Jon Masamitsu wrote:
> Please put a hold on these latest webrevs.  I will be updating
> them.
>
> Jon
>
> On 07/20/2016 11:06 AM, Jon Masamitsu wrote:
>>
>>
>> On 07/13/2016 04:11 AM, Thomas Schatzl wrote:
>>> Hi Jon,
>>>
>>> On Thu, 2016-06-23 at 13:32 -0700, Jon Masamitsu wrote:
>>>> The change has grown some (small now, not extra small).
>>>>
>>>> Added logging for the case where thread creation fails.
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_00_01a/
>>>>
>>>> Added fixes for failed thread creation.
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_00_01b/
>>>>
>>>> Complete fix with test added.
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8159073/webrev.01/
>>>>
>>>> Tested with gc_test_suite with new diagnostic flag on and off.
>>>    some comments:
>>>
>>> - in a few files (concurrentMarkSweepGeneration.cpp,
>>> parnewGeneration.cpp) there is a comment like:
>>>
>>> 2891       // If the total workers is greater than 1, then multiple
>>> workers
>>> 2892       // may be used at some time and the initialization has been
>>> set
>>> 2893       // such that the single threaded path cannot be used.
>>> 2894       if (workers->total_workers() > 1) {
>>>
>>> Wouldn't it be easier to just remove the special case for running the
>>> task in the VM thread?
>>>
>>> That would simplify this code, and probably the initialization code a
>>> bit.
>>> Not sure if we are concerned about this code now though.
>>
>> I have a patch that takes out  this special case but was
>> not sure if you wanted it in this patch.  Should I include it
>> (it's not in the webrev below).
>>>
>>> - I like the rename from set_active_workers() to
>>> update_active_workers().
>>>
>>> - sorry to always be the one complaining about this, but in
>>> workerManager.hpp indentation is off in several places:
>>>
>>>    63         log_trace(gc, task)("WorkerManager::add_workers() : "
>>>    64            "creation failed due to failed allocation of native
>>> %s",
>>>    65            new_worker == NULL ?  "memory" : "thread");
>>>
>>>    66         if (new_worker != NULL) {
>>>    67            delete new_worker;
>>>    68         }
>>>
>>>    78     log_trace(gc, task)("WorkerManager::add_workers() : "
>>>    79        "created_workers: %u", created_workers);
>>
>> I've fixed the white space but it is not showing up in
>> the webrev unless you look at the "patch" link.
>>
>>>
>>> (Also I think at least the last one conflicts with the recent changes
>>> to the log messages).
>>>
>>> - WorkGang::run_task(): from my understanding that method has always
>>> been intended to be a shortcut for
>>>
>>> old_num_workers = num_workers;
>>> update_workers(num_workers);
>>> run_task(cl);
>>> update_workers(old_num_workers);
>>>
>>> This is not the case here any more. The code silently changes the
>>> number of dispatched workers, which may lead to confusion. I.e. the
>>> caller may expect that the given do_work() closure method will be
>>> called with arguments from 0..num_workers-1 eventually.
>>>
>>> That is not the case here any more.
>>>
>>> I am not sure whether this is a problem, I think that all currently
>>> implemented closures are okay with that. But I am not sure what the
>>> problem is with the original code, so I would tend to try to avoid such
>>> a change in behavior. Again, the worst thing that may happen is that a
>>> single worker thread may get his do_work() method called multiple
>>> times.
>>>
>>> This is something that has been possible, and the code had to prepared
>>> for that, since forever.
>>>
>>> [Additionally, currently, all existing users of that run_task() method
>>> pass (or should pass) num_workers <= active_workers() that call this
>>> method, so no extra threads should be created.]
>>>
>>> The only problem calling update_active_workers() in that method right
>>> now is that it only allows less active workers than total workers if
>>> UseDynamicNumberOfGCThreads is true as far as I can see.
>>>
>>> So I tried a vm.gc test run with the patch at http://cr.openjdk.java.ne
>>> t/~tschatzl/8159073/webrev/hotspot.patch (basically your patch, with
>>> changes in WorkGang::run_task(), and removing the nasty assert in
>>> update_active_workers()), and it passed.
>>>
>>> I think that would be exactly the intention of the original change.
>>>
>>> What do you think?
>>
>> I incorporated the above change.
>>
>> Delta: http://cr.openjdk.java.net/~jmasa/8159073/webrev_delta_01_02/
>> Full: http://cr.openjdk.java.net/~jmasa/8159073/webrev.02/
>>
>> Thanks.
>>
>> Jon
>>
>>>
>>> Thanks,
>>>    Thomas
>>>
>>
>