RFR: JDK-8138717: TestGCEventMixedWithG1ConcurrentMark.java fails

[Bengt Rutisson]

Hi David,

On 2015-10-08 16:15, David Lindholm wrote:
> Bengt,
>
> The change looks good for fixing the test failure. Reviewed.

Thanks for looking at this!

Bengt

>
> But we should really have the discussion about what such a test can 
> expect.

Agreed. But in this case the test actually detected a change in behavior 
that I introduced. So, I think I should have been more careful in 
testing before I pushed and updated the test or reverted to the old 
behavior (as I did now).

Bengt

>
>
> Thanks,
> David
>
> On 2015-10-08 12:53, Bengt Rutisson wrote:
>>
>> Hi all,
>>
>> Could I have a couple of reviews for this fix?
>>
>> http://cr.openjdk.java.net/~brutisso/8138717/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8138717
>>
>> The fix for JDK-8134953 tried to keep creation of new GC ids the same 
>> as before. But there was one exception. When we create heap dumps or 
>> do heap inspection before or after a GC we no longer create a new GC 
>> id for those events. Instead they use the same GC id as the 
>> "enclosing" GC.
>>
>> This was an intentional change. But in retrospect it should probably 
>> not have been done as part of the other changes since
>> it breaks some tests that expect the GC id for the heap inspections 
>> to be different than the GC id for the actual GC. It should be a 
>> separate discussion of whether or not we should have different GC ids 
>> for these activities.
>>
>> Here's the change to collectedHeap.cpp that removes the creation of 
>> new GC ids:
>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/diff/983c56341c80/src/share/vm/gc/shared/collectedHeap.cpp 
>>
>>
>> The proposed patch adds the creation of new GC ids back at the same 
>> places. The patch also introduces a GCIdMarkAndRestore class since 
>> the "GC" for a heap dump or heap inspected will be nested inside of a 
>> normal GC. This is the same behavior as before JDK-8134953, but it 
>> means that we can't use a GCIdMark since that would reset the GC id 
>> to undefinied in its destructor.
>>
>> In fact there is one more place where we nest GC ids. It's in 
>> GenCollectedHeap::do_collection(). This isn't a real problem (yet) 
>> since no one is using the GC id after it has been set to undefinied. 
>> But now that we have the GCIdMarkAndRestore anyway, we can use that 
>> here too to have the correct GC id available at all times.
>>
>> Thanks,
>> Bengt
>