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

[Jon Masamitsu]

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
>