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

[Bengt Rutisson]

Thanks for looking at this, Jesper!

Bengt

On 2015-10-06 15:14, Jesper Wilhelmsson wrote:
> 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
>>
>>