Thumbs up Mandy
> On Nov 27, 2013, at 1:15 PM, Jaroslav Bachorik <jaroslav.bacho...@oracle.com> > wrote: > > Thanks for looking at this, Mandy. > > Here is the patch with added explanatory comments - > http://cr.openjdk.java.net/~jbachorik/6987597/webrev.05 > > -JB- > >> On 27.11.2013 21:33, Mandy Chung wrote: >>> On 11/27/2013 10:41 AM, Jaroslav Bachorik wrote: >>> Hi, >>> >>> I've uploaded the patch with the minimal changes that should resolve >>> this particular problem. >>> >>> http://cr.openjdk.java.net/~jbachorik/6987597/webrev.04 >> >> Looks good. I'm happy to see this simple change resolves the issue. It >> would be useful to add a comment before you push to explain why >> -XX:+ExplicitGCInvokesConcurrent flag is used for simplicity even though >> this flag is ignored by non-concurrent GC. >> >> Mandy >> >>> -JB- >>> >>>> On 22.11.2013 14:57, Jaroslav Bachorik wrote: >>>>> On 21.11.2013 17:51, Mandy Chung wrote: >>>>> Hi Jaroslav, >>>>> >>>>>> On 11/19/2013 6:23 AM, Jaroslav Bachorik wrote: >>>>>> Please, review this test fix. >>>>>> >>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-6987597 >>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/6987597/webrev.03 >>>>>> >>>>>> The fix moves the execution of the test routine to the >>>>>> GarbageCollectorMXBean notification handler - at that moment it's >>>>>> certain that GC has already happened and it is safe to assert the data >>>>>> provided by the MBean. >>>>> >>>>> This patch may have a potential issue when GC happens during the >>>>> process >>>>> of adding the notification handlers such as the number of increments is >>>>> greater than the number of registered handlers. By the time the main >>>>> thread reaches p.arriveAndAwaitAdvance, the phaser has advanced to the >>>>> next phase with few arrived parties and will wait forever (until the >>>>> next GC happens). This should rarely happen though. Note that >>>>> p.arriveAndAwaitAdvance continue to run even if the thread is >>>>> interrupted and what happens if jtreg attempts to time out the test? >>>> >>>> Thanks for catching this. It seems that using a Semaphore with adding >>>> permissions when a GC notification arrives would be more stable. >>>> >>>>> >>>>> Does this issue only happen to CMS with background GC thread? The >>>>> proposed patch seems a little overkill. I wonder if the test should >>>>> skip if running in CMS background mode. Does >>>>> -XX:+ExplicitGCInvokesConcurrent flag will get System.gc() to run in >>>>> foreground mode in CMS (I think that may get the GC to complete before >>>>> System.gc() returns?) >>>> >>>> Yes, currently only CMS. It seems that the semantics of System.gc() >>>> ("When control returns from the method call, the Java Virtual Machine >>>> has made a best effort to reclaim space from all discarded objects.") >>>> doesn't apply to the CMS collector. >>>> >>>> Adding "-XX:-ExplicitGCInvokesConcurrent" should force System.gc() to >>>> wait till the GC has been performed. This could help resolve this >>>> particular problem. >>>> >>>> -JB- >>>> >>>>> >>>>> Mandy >
