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

[Jon Masamitsu]

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

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

Wow.  I have not known that G1 logging is printed when PrintGCDetails is
off.  Is that  really the intent?

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