RFR: JDK-8138707, 8138862, 8138863: TestPromotionEventWithParallelScavenge.java crashes using undefined GC id.

[Bengt Rutisson]

Hi everyone,

I'd like to ask for a couple of reviews for these three changes. I chose 
to put all three webrevs in a single review email since they are closely 
related.

Originally I just wanted to address JDK-8138707 which is caused by a 
recent change I made that stores the current GC id in the working 
thread. In that fix I missed adding the GC id to the ParallelGC worker 
threads. The fix for that is pretty simple, we can do it the same way 
that it is done for AbstractGangTask and similar. That is, store the 
current GC id in the task and then let the worker thread pick the GC id 
up from the task when it starts executing a task.

When I did that simple fix I realized that there are some tweaks in the 
ParallelGC version of the task system that made this a bit tricky. There 
is a NoopTask that is a singleton and thus gets set up before we have an 
reasonable GC id to use. That is not such a big problem since a NoopTask 
will not do any work for a GC (by definition) and therefore does not 
need to log or send events using a GC id.

A more problematic thing is that there is a WaitForBarrierGCTask that is 
used by the worker threads when they are idle and need to wait for work 
to appear. They actually use an IdleTask for waiting. This task is 
created "normally" and can access the gc_id field in the worker thread. 
But the IdleTasks all use a singleton WaitForBarrierGCTask for 
notification purposes. However, they actually don't use it as a task. 
They just use it as a convenient wrapper for a monitor. But the 
singleton setup of a WaitForBarrierGCTask is a problem since there is no 
GC id to use at the initialization time.

In GCTaskManager::execute_and_wait() the WaitForBarrierGCTask is used as 
a normal GCTask and is created in a way that allows the GC id to be set 
properly.

To avoid having to handle two different use cases for 
WaitForBarrierGCTask I factored out the monotor support into a 
WaitHelper class. That can be used without GC id issues by the IdleTasks 
and the WaitForBarrierGCTask can simply aggregate such a helper.

While trying to understand all this I stumbled over some unused code. 
So, I ended up splitting this work up into three changesets:

JDK-8138862: Remove some unused code and subclasses in gcTaskManager.hpp/cpp
https://bugs.openjdk.java.net/browse/JDK-8138862
http://cr.openjdk.java.net/~brutisso/8138862/webrev.00/

JDK-8138863: Refactor WaitForBarrierGCTask
https://bugs.openjdk.java.net/browse/JDK-8138863
http://cr.openjdk.java.net/~brutisso/8138863/webrev.00/

JDK-8138707: TestPromotionEventWithParallelScavenge.java crashes using 
undefined GC id.
https://bugs.openjdk.java.net/browse/JDK-8138707
http://cr.openjdk.java.net/~brutisso/8138707/webrev.00/

There are some descriptions in the JBS issues about what I did. Here are 
a few more details:

JDK-8138862: Remove some unused code and subclasses in gcTaskManager.hpp/cpp
- Since I removed the BarrierGCTask I renamed the "kind" from 
barrier_task to wait_for_barrier_task.
- The NotifyDoneClosure was only set up in the GCTaskManager::create(int 
workers, NotifyDoneClosure* ndc) method, but this was never used. So 
effectively the _ndc field of GCTaskManager was always NULL.
- By removing BarrierGCTask there was no longer a need for 
WaitForBarrierGCTask::do_it_internal() since all the work can be done in 
the do_it() method.
- I'm sure there is more dead and unused cde in gcTaskManager.hpp/cpp. I 
removed the code that was obscuring things for the changes that I needed 
to do. There may well be more code that can be deleted and cleaned up later.

JDK-8138863: Refactor WaitForBarrierGCTask
- By splitting out the WaitHelper parts from WaitForBarrierGCTask there 
is no longer anyone setting it up as a C-heap object. Thus, that factory 
method and the instance state could be removed.

JDK-8138707: TestPromotionEventWithParallelScavenge.java crashes using 
undefined GC id.
- The fix here is very similar to what is done in for example 
AbstractGangTask and the GangWorkers. See:
http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/d9d44c9d7bf0/src/share/vm/gc/shared/workgroup.hpp#l56
http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/d9d44c9d7bf0/src/share/vm/gc/shared/workgroup.cpp#l329


Thanks,
Bengt