> On Oct 19, 2017, at 12:39 PM, Roman Kennke <rken...@redhat.com> wrote: > > Am 18.10.2017 um 22:41 schrieb Kim Barrett: >>> On Oct 18, 2017, at 4:04 PM, Roman Kennke <rken...@redhat.com> wrote: >>> >>> Am 18.10.2017 um 20:41 schrieb Kim Barrett: >>>>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <rken...@redhat.com> wrote: >>>>> Differential webrev: >>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ >>>>> <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/> >>>>> >>>>> Full webrev: >>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ >>>>> <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/> >>>>> >>>>> Better now? >>>>> >>>>> Thanks, Roman >>>> Looks good. >>>> >>> Hi Kim, >>> >>> thanks for the review. >>> >>> I just fixed a bug caused by my similar CMSHeap extraction, and I think I >>> need to do the same thing for SerialHeap too: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8189373 >>> >>> This is the fix for the CMSHeap issue: >>> >>> http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ >>> <http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/> >>> >>> I'll do the same for SerialHeap once the above has been approved and >>> pushed, otherwise it'll be a mess. ;-) >>> >>> Roman >> The SA strikes again! Yes, it looks like the same thing should be done for >> SerialHeap. >> I’m going to leave the review of 8189373 to others who have more clue about >> the SA. >> > Okidoki, so here comes the SerialGC with SA boilerplate: > > Differential: > http://cr.openjdk.java.net/~rkennke/8183542/webrev.02.diff/ > <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02.diff/> > > Full: > http://cr.openjdk.java.net/~rkennke/8183542/webrev.02/ > <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02/> > > This builds on top of the patch for > https://bugs.openjdk.java.net/browse/JDK-8189373 which should land in the > repo shortly, and implements the same thing for SerialHeap. It also passes > the test that failed in the mentioned bug report (with -XX:+UseSerialGC). > > Can I get reviews (for the changed/added stuff) again? > > Thanks, Roman
Looks good to me, though I don’t know much about the SA. It at least looks consistent with the changes for JDK-8189373.
