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

[Mikael Gerdin]

Hi Jon,

On 2016-02-18 22:49, Jon Masamitsu wrote:
>
>
> 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.

Thanks Jon.
I'll go ahead and integrate this change then.

/Mikael

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