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

[Mikael Gerdin]

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