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

[Bengt Rutisson]

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...

>
>>
>> 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.

>
> 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.
>>>
>>
>