Request for review (s) : 8017462: G1: guarantee fails with UseDynamicNumberOfGCThreads
jon.masamitsu at oracle.com
Tue Mar 31 16:49:40 UTC 2015
On 3/30/2015 4:03 AM, Bengt Rutisson wrote:
> Hi Jon,
> On 2015-03-27 15:37, Jon Masamitsu wrote:
>> 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)
>>> 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
>>> 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",
>>> 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",
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
Latest delta webrev (only the test was changed).
>> 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.
>>>> On 3/10/2015 9:26 AM, Jon Masamitsu wrote:
>>>>> 8017462: G1: guarantee fails with UseDynamicNumberOfGCThreads
>>>>> 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.
>>>>> Tested with gc_test_suite.
More information about the hotspot-gc-dev