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

[Jon Masamitsu]

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.

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