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

[Jon Masamitsu]

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