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

[Jon Masamitsu]

On 03/11/2015 05:07 AM, Bengt Rutisson wrote:
>
> 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/

I'll adopt your patch.  Thanks.


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

I see you wisely pushed your change already.

Jon
>
> Thanks,
> Bengt
>
>>
>> Thanks,
>>    Thomas
>>
>>
>