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

[Jon Masamitsu]

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