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

[Bengt Rutisson]

Hi again,

An update on the last patch. The one for JDK-8138707. Instead of having 
the initialize method switch basted on the kind of the subclasses to set 
the GC id I thought it would be better to let the NoopGCTask subclass 
pass the undefinied GC id to the call to the super constructor.

This meant that I added another constructor to GCTask. It already had 4 
constructors. But two of them were unused, so with this patch we are 
actually down to 3 constructors.

I also moved the initialization of all fields into the initialize() method.

Updated webrev:
http://cr.openjdk.java.net/~brutisso/8138707/webrev.01/

Diff compared to last version (but all of it changed, so I think the 
above link is more useful):
http://cr.openjdk.java.net/~brutisso/8138707/webrev.00-01.diff/

Thanks,
Bengt

On 2015-10-05 11:44, Bengt Rutisson wrote:
>
> 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
>
>