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

[kirk.pepperdine at gmail.com]

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

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