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

[Mikael Gerdin]

Hi Bengt,

On 2015-10-06 14:10, Bengt Rutisson wrote:
>
> 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/

Looks good.

>
> 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/

Looks good.

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

In gcTaskManager.cpp:

  382   _wait_helper() {

Explicitly constructing this member using the default constructor is 
unnecessary, it'll be called for you.
I don't need to re-review this change after it's gone.

/Mikael

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