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

[Thomas Schatzl]

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

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

Thanks,
  Thomas