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

[Thomas Schatzl]

Hi,

On Fri, 2018-12-07 at 22:42 +0800, Man Cao wrote:
> 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.

Given this explanation it seems okay to use the extra classes.

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

We can discuss this in a separate RFE.

> 
> > 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/resourc
> > es/aliasmap). 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.

Creating a CSR and getting it approved is not a big deal; it may even
be useful as it clearly communicates the change to the users.

Additionally I think due to that translation table I mentioned, the old
name can still be used I think.

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

Thanks,
  Thomas