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

[Bengt Rutisson]

Hi Mikael,

Thanks a lot for the review!

I'll remove the explicit initialization of _wait_helper().

Bengt

On 2015-10-06 14:26, Mikael Gerdin wrote:
> 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
>>>
>>>
>>
>