RFR (M) 8149013: Remove unused and dead code from G1CollectorPolicy

[Jon Masamitsu]

On 02/18/2016 07:51 AM, Mikael Gerdin wrote:
> Jon,
>
> On 2016-02-08 19:57, Jon Masamitsu wrote:
>>
>>
>> On 02/05/2016 08:46 PM, kirk.pepperdine at gmail.com wrote:
>>>> On Feb 5, 2016, at 10:47 PM, Jon Masamitsu <jon.masamitsu at oracle.com>
>>>> wrote:
>>>>
>>>>
>>>>
>>>> On 2/5/2016 5:25 AM, Mikael Gerdin wrote:
>>>>> Hi Jon,
>>>>>
>>>>> On 2016-02-04 19:41, Jon Masamitsu wrote:
>>>>>>
>>>>>> On 02/04/2016 05:02 AM, Mikael Gerdin wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Here's a cleanup of dead and unused code from the G1 collector
>>>>>>> policy.
>>>>>>>
>>>>>>> Some unused member variables can be removed:
>>>>>>> _no_of_gc_threads (only getter/setter)
>>>>>>> _parallel_gc_threads (only used locally in constructor)
>>>>>>>
>>>>>>>
>>>>>>> I'd also suggest to remove functionality related to
>>>>>>> Trace{Young,Old}GenTime from G1. The statistics collected are 
>>>>>>> usually
>>>>>>> too coarse (avg. pause times over a complete run) to be useful and
>>>>>>> they have not been kept in sync with the time tracking done through
>>>>>>> the phase times tracking object.
>>>>>> Mikael,
>>>>>>
>>>>>> One of the  things that we don't have at the moment is an easy
>>>>>> way for non-GC engineers to measure GC pauses.  Yes, the gclogapp
>>>>>> and the new aurora reading of the jfr events for pause time is a way
>>>>>> but not as easy a way as having the GC dump the statistics at the
>>>>>> end of the run.   Should we consider fixing and retaining this the
>>>>>> Trace{Young,Old}GenTime to help others help us?  Or is there
>>>>>> something we should implement instead?
>>>>> I did put some thought into the state of Trace{Young,Old}GenTime
>>>>> before suggesting their removal. I'm not sure that reporting the
>>>>> average pause time over an entire run at VM termination is something
>>>>> that will help that much.
>>>>> It does not account for things like:
>>>>> * mixed gcs versus young gcs
>>>>> * warmup phases
>>>>> * outliers
>>>> I agree that much is mixed together in the summaries.   For a GC
>>>> developer it
>>>> would not be that useful.  The only good reason for keeping it is
>>>> that it is
>>>> easy for a non-GC developer to look at.
>>> There is so much variation in pause times in real world applications
>>> across all phases of the run time that IMHO, there is very little, if
>>> any value in looking at average pause time. Pause times simply don’t
>>> band in a way that would give you useful information.
>>
>> I'd like to catch the 20% systematic pause time regression.
>> Catching the smaller ones would be nice but, as you say,
>> the variation in pauses makes that a harder job.
>
> Do you feel that using -XX:+TraceYoungGenTime would be your first 
> choice of tool for catching 20% systematic pause time regressions in G1?

It would not be my first choice but I look at GC output
enough to have other options.  I think that we are developing
other options for monitoring GC pauses so if you would like
to delete this, I withdraw any objections I had.

Jon

>
> /Mikael
>
>>
>> Jon
>>
>>
>>
>>>
>>> Kind regards,
>>> Kirk
>>>
>>>>> I wonder if any end users are actually interested in the average gc
>>>>> pause time over an entire VM lifetime.
>>>>>
>>>>> I also think it's problematic that the set of sub-phases which are
>>>>> recorded in the TraceYoungGenTime data is an old copy of the set of
>>>>> sub-phases present in the phase times object.
>>>>> If we feel that maintaining running averages and standard deviations
>>>>> of sub-phases is important then it would probably be more reasonable
>>>>> to integrate that code more tightly with the phase times object 
>>>>> itself.
>>>>>
>>>>> If you feel that part of the functionality needs to be retained,
>>>>> would you agree to just keeping the pause time tracking and dropping
>>>>> the sub-phase tracking, thus making it behave similar to how other
>>>>> collectors report when the flags are enabled?
>>>> Pause time tracking would be enough.  If a change causes pauses to
>>>> increase by 50%,
>>>> I'm not sure it would get noticed (since performance regression are
>>>> tracked by
>>>> benchmark scores and it often takes a lot of bad GC to affect a
>>>> benchmark score).
>>>>
>>>> Thanks.
>>>>
>>>> Jon
>>>>
>>>>> /Mikael
>>>>>
>>>>>> Jon
>>>>>>
>>>>>>> Removing this functionality from G1 leads to removal of:
>>>>>>>
>>>>>>> _stop_world_start (only used by Trace*GenTime)
>>>>>>> record_concurrent_pause()
>>>>>>> print_tracing_info()
>>>>>>> record_stop_world_start()
>>>>>>> etc.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8149013
>>>>>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8149013/webrev.0/
>>>>>>> Testing: Local build and local GCBacsher
>>>>>>>
>>>>>>> Thanks
>>>>>>> /Mikael
>>