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
