RFR (M): 8212206: Refactor AdaptiveSizePolicy to separate out code related to GC overhead

[Thomas Schatzl]

Hi Man,

  first, sorry for the wait, this RFR fell through the cracks somehow
here...

On Fri, 2018-11-16 at 19:10 -0800, Man Cao wrote:
> Hi all,
> 
> Could I have reviews for this refactoring change?
> Webrev: https://cr.openjdk.java.net/~manc/8212206/webrev.00/
> RFE: https://bugs.openjdk.java.net/browse/JDK-8212206
> 
> It basically moves AdaptiveSizePolicy::check_gc_overhead_limit() to
> its own class so it can be shared with collectors that do not support
> AdaptiveSizePolicy.
> Each collector can provide its own functions to check if GC time or
> space overhead has exceeded threshold. It also removes one unused
> parameter "young_live".
> I think this is sufficient to share the common code for implementing
> UseGCOverheadLimit for G1 (JDK-8212084).
> 
> Initially I tried moving all code for collecting GC time overhead in
> AdaptiveSizePolicy to a separate class, e.g.
> <minor|major>_collection_<begin|end>().
> However, G1 already has its own ways to compute GC cost in
> G1Analytics. For example, G1HeapSizingPolicy::expansion_amount()
> already uses G1Analytics::_recent_avg_pause_time_ratio for GC cost.
> It is undesirable to have two ways to compute GC cost in one
> collector.
> In general, different collectors have different ways to compute GC
> time and space overhead. It could be too restrictive to make an
> interface to define how to collect GC time and space overhead.
> 
> Thus, this patch only moves the code to check GC overhead in a common
> place. It could be reused to implement UseGCOverheadLimit for
> collectors such as ZGC in the future.

- I would prefer if "OverheadChecker" would be prefixed by a "GC", i.e.
GCOverheadChecker to make sure it is about GC

- not sure about the usefulness of the OverheadTester interface. At
least in the current implementation its implementation could be easily
replaced by static functions returning a bool. That would save a lot of
boiler plate code. Is there some reason to have explicit classes for
that?

At least please add a "GC" prefix to the class here too.

- style: in new classes, please align the visibility modifiers with the
"class" keyword without indentation.

- extra newline at adaptiveSizePolicy.cpp:489

looks good otherwise.

> 
> There could be further refactoring to do on top of this patch. I'd
> like to get some opinions on whether they are desirable to do, and if
> they could be done as part of this RFE:
> (1) The develop flag AdaptiveSizePolicyGCTimeLimitThreshold could be
> renamed to GCOverheadLimitThreshold to be more accurate. I'm not sure
> about the process of renaming a develop flag, though.

Just rename it. Develop flags can be changed without any additional
process.

> (2) The instance of the new class OverheadChecker could be a member
> of CollectedHeap, instead of a member of AdaptiveSizePolicy. Note
> that CollectedHeap::mem_allocate() already has a parameter
> "bool* gc_overhead_limit_was_exceeded".

Assuming that all collectors want to implement this, and actually need
to I am leaning towards doing so. However the ZGC/Shenandoah people
might object to this.

> (3) The hsperf counter sun.gc.policy.gcTimeLimitExceeded is currently
> only available to ParallelGC. It can be moved to GCPolicyCounters so
> it is available to all collectors. Also gcTimeLimitExceeded is a
> misnomer, gcOverheadLimitExceeded is more accurate.

Okay. However I do not know how to rename them and what are the
requirements here. Probably a CSR is needed, although I have seen
translation tables used (in
src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/resources/a
liasmap). I cc'ed hotspot-dev in the assumption somebody else there
knows more about this.

Thanks,
  Thomas