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

Thu May 22 11:02:22 UTC 2014

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140522/4ba3d71f/attachment.htm>