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

[Bengt Rutisson]

Hi Jon and Thomas,

On 2015-03-10 19:45, Thomas Schatzl wrote:
> Hi Jon,
>
> On Tue, 2015-03-10 at 09:26 -0700, 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.
> - I would somewhat prefer if _active_threads would not be stored in
> every WorkerDataArray. This seems a waste of space. G1GCPhaseTimes
> already stores it, so it could easily passed to
> WorkerDataArray::verify()/print() instead which seems cleaner to me.
>
> This would also avoid the additional parameter in
> note_string_dedup_fixup_start().
>
> - WorkerDataArray::reset() is PRODUCT_RETURN. That means that in a
> product VM, WorkerDataArray::_active_length is seemingly never assigned
> to then as far as I can see.
>
> - is it possible to create a test for G1 that runs with
> G1LogLevel=finest and parses and verifies output of one GC phase that
> takes at least some time? I.e. that you have active_workers amount of
> durations in that line and the sum matches more or less.
>
> - Bengt is currently also changing this code significantly (see the
> review for 8074037: Refactor the G1GCPhaseTime logging to make it easier
> to add new phases). Somebody will have to do a significant amount of
> merging :/

Right. This will conflict with my changes.

On the other hand I agree with Thomas that it doesn't really look good 
to store the active thread value in the WorkerDataArray. So, if we skip 
that and base the changes on top of my patch I think you can do a much 
less intrusive change.

Here's a webrev that also passes the test that you wrote 
(TestDynamicNumberOfGCThreads.java):

http://cr.openjdk.java.net/~brutisso/8017462/webrev.01/

It reuses the active thread value from the G1GCPhaseTimes instance and 
thus all changes can be contained within one file.

Jon, what do you think about this solution instead?

If we go with this, is it ok that I push my changes first?

Thanks,
Bengt

>
> Thanks,
>    Thomas
>
>