RFR: 8038925: Java with G1 crashes in dump_instance_fields using jmap or jcmd without fullgc

[Andreas Eriksson]

Great, thanks.

/Andreas

On 2014-05-23 12:37, Mikael Gerdin wrote:
> On Thursday 22 May 2014 13.39.13 Andreas Eriksson wrote:
>> Hi,
>>
>> Bengt:
>>
>> Right, that should be enough, thanks.
>>
>> Mikael:
>>
>> Can I use you as a reviewer for this latest version as well?
> Yes, this looks fine.
>
> /Mikael
>
>> Regards,
>> Andreas
>>
>> On 2014-05-22 13:02, Bengt Rutisson wrote:
>>> Hi Andreas,
>>>
>>> This is a HotSpot change and for JDK7 HotSpot was developed in the hsx
>>> project. I am a Reviewer in the hsx project
>>> (http://openjdk.java.net/census#brutisso) isn't that enough to review
>>> this change?
>>>
>>> Anyway, last webrev looks good. Thanks for fixing the test!
>>>
>>> Bengt
>>>
>>> On 5/22/14 10:43 AM, Andreas Eriksson wrote:
>>>> Hi,
>>>>
>>>> Adding jdk7u-dev.
>>>>
>>>> Could I have a jdk7u Reviewer look at this as well please? This is a
>>>> jdk7 only fix.
>>>> (There is a related problem in jdk8 and jdk9, where an assert can
>>>> fail because of this problem. I have logged another bug for this.)
>>>>
>>>> Description:
>>>> Due to the marking cleanup reclaiming empty regions, and having stale
>>>> references a crash can occur when doing a heap dump.
>>>> The code tries to do an is_klass check on the object, which crashes
>>>> the VM.
>>>> The fix is to add an is_perm check before doing the check, since
>>>> is_perm will do a bounds check on the oop and if it's in the perm gen
>>>> we know it's safe to look at it since G1 only ever does full
>>>> compactions of the perm gen.
>>>>
>>>> For more information, and a more in-depth analysis, please see the
>>>> jira bug.
>>>>
>>>> http://cr.openjdk.java.net/~aeriksso/8038925/webrev.03/
>>>> https://bugs.openjdk.java.net/browse/JDK-8038925
>>>>
>>>> Regards,
>>>> Andreas
>>>>
>>>> On 2014-05-22 10:14, Andreas Eriksson wrote:
>>>>> OK, I'll remove it.
>>>>>
>>>>> Thanks,
>>>>> Andreas
>>>>>
>>>>> On 2014-05-22 10:02, Bengt Rutisson wrote:
>>>>>> Hi Andreas,
>>>>>>
>>>>>> On 5/21/14 2:05 PM, Andreas Eriksson wrote:
>>>>>>> A new webrev is up:
>>>>>>> http://cr.openjdk.java.net/~aeriksso/8038925/webrev.02/
>>>>>>>
>>>>>>> The test now verifies that no full GC has been done after doing
>>>>>>> the heap dump.
>>>>>>> I also modified the test to not run if it for some reason is not
>>>>>>> using G1.
>>>>>> Thanks for the update, Andreas.
>>>>>>
>>>>>> The test looks good except for the @run tag.
>>>>>>
>>>>>> @run main/othervm -Xms512m -Xmx1024m -XX:+UseG1GC
>>>>>> -XX:+ExplicitGCInvokesConcurrent TestG1ConcurrentGCHeapDump
>>>>>>
>>>>>> The problem is that more GC flags will be added when the test is
>>>>>> run in nightly testing. Some GC flags will conflict with UseG1GC.
>>>>>> On the other hand at some point UseG1GC will be one of the flags
>>>>>> that is added.
>>>>>>
>>>>>> So, I think what you need to do is just remove "-XX:+UseG1GC" from
>>>>>> the @run tag. Then your test will report log "skipped" when it is
>>>>>> run in the nightly testing except for the nightly testing done with
>>>>>> G1 where it will actually do something.
>>>>>>
>>>>>> Bengt
>>>>>>
>>>>>>> Regards,
>>>>>>> Andreas
>>>>>>>
>>>>>>> On 2014-05-21 12:31, Bengt Rutisson wrote:
>>>>>>>> On 5/21/14 12:12 PM, Andreas Eriksson wrote:
>>>>>>>>> Hi Bengt, thanks for looking at this.
>>>>>>>>>
>>>>>>>>> I agree that verifying that no full GC has happened would be good.
>>>>>>>>> One thing I see as problematic though, is what to do if a full
>>>>>>>>> GC has happened.
>>>>>>>>> Should I make the test fail? Or is there some way to signal that
>>>>>>>>> the test was inconclusive / couldn't finish?
>>>>>>>> Personally I would prefer to make the test fail. JTreg only has
>>>>>>>> two states, PASS or FAIL, and I think that if we allow it to pass
>>>>>>>> we might not notice if there is some change that makes the test
>>>>>>>> always get a full GC and then never actually testing anything.
>>>>>>>>
>>>>>>>> I don't think it will fail intermittently by getting full GCs. I
>>>>>>>> think the test is pretty stable. But I think it would be good to
>>>>>>>> have a way of noticing if it stops testing what it is supposed to
>>>>>>>> test.
>>>>>>>>
>>>>>>>> (By the way, I would really like a "SKIPPED" state in JTreg but I
>>>>>>>> haven't had any luck pushing for that. I think it could be useful
>>>>>>>> for other things as well. There is for example no good reason for
>>>>>>>> your test to be run 4 times each night with the exact same
>>>>>>>> binary. But that is what happens since we can't say that we
>>>>>>>> should skip this test if we run with any other GC than G1.)
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Bengt
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Andreas
>>>>>>>>>
>>>>>>>>> On 2014-05-21 11:55, Bengt Rutisson wrote:
>>>>>>>>>> Hi Andreas,
>>>>>>>>>>
>>>>>>>>>> The fix looks good.
>>>>>>>>>>
>>>>>>>>>> One comment about the test. It does not verify that no full GC
>>>>>>>>>> happens. The way the test is set up I guess that should not
>>>>>>>>>> happen and I am not sure it is worth the effort to add a check
>>>>>>>>>> for it. Just wanted to mention it if you want to make test more
>>>>>>>>>> resilient to future changes in the JVM that for some reason can
>>>>>>>>>> trigger a full GC for this test.
>>>>>>>>>>
>>>>>>>>>> I'm fine with leaving the test as it is.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Bengt
>>>>>>>>>>
>>>>>>>>>> On 5/20/14 4:26 PM, Andreas Eriksson wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> Could I have a review for this G1 jdk7 only fix please?
>>>>>>>>>>> (There is a related problem in jdk8 and jdk9, where an assert
>>>>>>>>>>> can fail because of this problem. I have logged another bug
>>>>>>>>>>> for this.)
>>>>>>>>>>>
>>>>>>>>>>> Description:
>>>>>>>>>>> Due to the marking cleanup reclaiming empty regions, and
>>>>>>>>>>> having stale references a crash can occur when doing a heap dump.
>>>>>>>>>>> The code tries to do an is_klass check on the object, which
>>>>>>>>>>> crashes the VM.
>>>>>>>>>>> The fix is to add an is_perm check before doing the check,
>>>>>>>>>>> since is_perm will do a bounds check on the oop and if it's in
>>>>>>>>>>> the perm gen we know it's safe to look at it since G1 only
>>>>>>>>>>> ever does full compactions of the perm gen.
>>>>>>>>>>>
>>>>>>>>>>> For more information, and a more in-depth analysis, please see
>>>>>>>>>>> the jira bug.
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8038925/webrev.01/
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8038925
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Andreas