Request for review (s) : 8017462: G1: guarantee fails with UseDynamicNumberOfGCThreads

Wed Apr 1 18:16:09 UTC 2015

Hi Jon,

Latest webrev looks good to me.

Thanks,
Bengt

On 31/03/15 18:49, Jon Masamitsu wrote:
>
> On 3/30/2015 4:03 AM, Bengt Rutisson wrote:
>>
>> Hi Jon,
>>
>> On 2015-03-27 15:37, Jon Masamitsu wrote:
>>> Bengt,
>>>
>>> Thanks for looking at this.
>>>
>>> On 3/27/2015 2:48 AM, Bengt Rutisson wrote:
>>>>
>>>> On 2015-03-27 05:33, Jon Masamitsu wrote:
>>>>> The fix for 8017462 has been changed to adopt the suggestion from
>>>>> Bengt (thanks, Bengt).  The patch from Bengt has been enhanced to
>>>>> include some of the changes in this first version that did not make
>>>>> it into Bengt's original patch.  The test remains the same. A CR 
>>>>> has been
>>>>> created to add another test to check the consistency of one of
>>>>> the per thread statistics with the sum of the statistics over all the
>>>>> threads (e.g., per thread object copy time and total object copy 
>>>>> time)
>>>>> https://bugs.openjdk.java.net/browse/JDK-8076140
>>>>>
>>>>> http://cr.openjdk.java.net/~jmasa/8017462/webrev.01/
>>>>
>>>> Thanks for adopting this patch. I think it looks good now. :)
>>>>
>>>> A couple of questions about the test case.
>>>>
>>>> The proposed test will run with any GC configuration since it just 
>>>> discards the JTreg configuration and selects its own GC. You have 
>>>> specified @requires vm.gc=="null", which makes sense since we don't 
>>>> have to run this test in every configuration. The test will anyway 
>>>> always run with the GCs that it specifies itself. However, the 
>>>> @requires tag will mean that the test is only run if no GC is 
>>>> specified. Currently there is a "noopt" baseline in our nightly 
>>>> testing that runs without explicitly setting a GC, so this test 
>>>> will be run there. But if we change the nightly baselines to always 
>>>> specify a GC we will never run this test.
>>>
>>> I would not expect that the nightly testing to always specify a 
>>> specific GC because
>>> we do have a default GC and test should be run when we do the 
>>> selection of the
>>> GC.  Is this going to changes such that every nightly test specifies 
>>> a specific GC?
>>>
>>> Are you suggesting that the command line be changed to not include a
>>> specific GC and that the the @requires be removed?  So the test is run
>>> with whatever GC is being tested?
>>
>> I think I prefer that the test specifies the GCs that it needs to 
>> test rather than relying on the framework to pass the relevant GC 
>> flags. So, I prefer the way the test is written now. My question was 
>> really if we want to use @requires for controlling when a test is 
>> run. More below...
>
> OK.
>
>>
>>>
>>>>
>>>> I guess this is a more general question. Should we use @requires to 
>>>> avoid running the same test many times or should we only use it 
>>>> when the test actually requires a specific GC configuration?
>>>
>>> I'll happily take guidance on this point.   Only using it when tests 
>>> actually requires a specific
>>> GC sounds OK.
>>
>> I'm also not sure, so my question was really a question. Is this how 
>> we want to make sure that a generic test is only run once? There are 
>> other ways...test groups, nightly batch configuration etc...
>>
>>>
>>>>
>>>> Also, the test use a lot of flags:
>>>>
>>>> "-XX:+UnlockExperimentalVMOptions", "-XX:G1LogLevel=finest", 
>>>> "-XX:+" + gcFlag, "-Xmx10M", "-showversion", "-XX:+PrintGCDetails", 
>>>> "-XX:+UseDynamicNumberOfGCThreads", "-XX:+TraceDynamicGCThreads"
>>>>
>>>>
>>>> I think this should be enough:
>>>>
>>>> "-XX:+" + gcFlag, "-Xmx10M", "-XX:+UseDynamicNumberOfGCThreads", 
>>>> "-XX:+TraceDynamicGCThreads"
>>>>
>>>> Is there a particular reason to have the other flags?
>>>
>>> In some of my testing there was a failure when I ran with log level 
>>> set to finest
>>> that I did not see when I used the default logging level.  I cannot 
>>> recall if it was
>>> actually this test but I decided to add finest to this test as a 
>>> result of seeing
>>> that failure.
>>
>> I don't see why the G1 log level would affect this test. Can you try 
>> to reproduce the issue? If not I would prefer to remove those flags.
>
> I removed the extra flags "-XX:+UnlockExperimentalVMOptions", 
> "-XX:G1LogLevel=finest"
>
> If I remove the "-XX:+PrintGCDetails" the test passes with a recent 
> build of gc_baseline.  If
> I keep that flag, the gc_baseline build fails (I added the stack trace 
> to the CR).   It passes with
> -XX:+PrintGCDetails and this fix.
>
> If the log level is not finer the method note_gc_end() is not called 
> and the failure happens in
> note_gc_end().
>
> Latest delta webrev (only the test was changed).
>
> http://cr.openjdk.java.net/~jmasa/8017462/webrev_delta_02/
>
> Full webrev
>
> http://cr.openjdk.java.net/~jmasa/8017462/webrev.02/
>
> Thanks.
>
> Jon
>
>>
>>>
>>> Wow.  I have not known that G1 logging is printed when 
>>> PrintGCDetails is
>>> off.  Is that  really the intent?
>>
>> Yes, that was intentional when G1LogLevel was introduced. I'm not 
>> saying it is the right choice, but it was intentional. All of this 
>> will be replaced with the unified logging for JDK 9, so I doubt that 
>> it is worth digging too deep into it now.
>>
>> Thanks,
>> Bengt
>>
>>>
>>> Jon
>>>
>>>
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>>
>>>>> On 3/10/2015 9:26 AM, Jon Masamitsu wrote:
>>>>>> 8017462: G1: guarantee fails with UseDynamicNumberOfGCThreads
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8017462
>>>>>>
>>>>>> When fewer than the maximum number of threads was being used for
>>>>>> a parallel section, phase times for the threads that did not 
>>>>>> execute and
>>>>>> averages for the phase were misleading.  The fix passes the 
>>>>>> active number of
>>>>>> GC threads  to the G1 phase timers.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jmasa/8017462/webrev.00/
>>>>>>
>>>>>> Tested with gc_test_suite.
>>>>>
>>>>
>>>
>>
>