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

[Jesper Wilhelmsson]

All three changes are fine.
Go for it!
/Jesper

Den 5/10/15 kl. 11:44, skrev 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
>
>