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

[Man Cao]

Hi Thomas,

Thanks for the review. No worries about the delay. Responding to some
comments below, will address other comments soon.

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

The OverheadTester interface provides a way to pass functions as parameters
to OverheadChecker::check_gc_overhead_limit(), and hide details about what
parameters are required to compute is_exceeded().
We could change OverheadChecker::check_gc_overhead_limit() to take "bool
time_overhead_exceeded, bool space_overhead_exceeded" parameters instead of
two pointers to OverheadTester.
Then we can have two static functions instead of the OverheadTester interface.
However, it would introduce a small behavior change for
AdaptiveSizePolicy::check_gc_overhead_limit().
Specifically, it would compute whether time and space overhead limits are
exceeded every time the function is called. Currently it only needs to
compute these under the following condition:
 !GCCause::is_user_requested_gc(gc_cause) &&
!GCCause::is_serviceability_requested_gc(gc_cause) && is_full_gc
If this behavior change is OK, I can replace the OverheadTester interface.

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.

I think it is reasonable for all collectors to support this, but the
low-pause collectors may have a very different definition for GC overhead.
I also received internal user complaints that the current
UseGCOverheadLimit for ParallelGC and CMS is too difficult to get
triggered, because of the requirement for 5 consecutive full GCs.
It might also be a problem for G1, as it avoids full GCs eagerly. I think
we might need to change the 5-full-GC requirement in the future to make it
more useful.

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.

If the process is too complicated, I can live with the current imperfect
name. It is not causing much ambiguity. I can just move it's definition to
GCPolicyCounters.
Keeping its name unchanged also reduces users' work when they upgrade to
new JDK, if they are already monitoring this counter using their own tools.
In fact, the more useful counter for monitoring purpose is the raw value
for gc_cost().
We have a local patch to export it for ParallelGC and CMS, but it doesn't
work for G1 yet. Perhaps we can upstream it in the future when it works for
G1.

-Man
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181207/7cd3115e/attachment.htm>