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

[Mikael Gerdin]

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?

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