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

[Man Cao]

Friendly ping.

-Man


On Sat, Nov 17, 2018 at 11:10 AM Man Cao <manc at google.com> 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.
>
> 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.
> (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".
> (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.
>
> Thanks,
> Man
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181206/41f8db42/attachment.htm>