RFR: Performance improvements for non-generational modes

[Y. Srinivas Ramakrishna]

The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.

----------------------------------------------------------------------
On Sat, 18 Feb 2023 01:56:10 GMT, William Kemper <wkemper at openjdk.org> wrote:

> There are a basket of changes here:
> * Make heuristics a bit less aggressive:
>    * Revert threshold triggers to use `available` rather than adjusted head room.
>    * Remove code that adjusted average gc time, it was causing wild fluctuations and making heuristic behave erratically
> * Skip over more generational mode constraints on the allocation path
> * Disable `ShenadoahEvacLABRatio` by default as it causes unused bytes in survivors regions, which in turn leads to these regions to being included in the mutator view of free memory. This contributed to higher fragmentation rates.
> * More detailed debugging messages under the `gc+free` tags.

Sorry to not get these comments back in time.

I didn't understand some of the changes, although the majority seemed reasonable.

I just had a few comments where the logging, control flow, and comments seemed to have become inconsistent (or at least confusing to the naive reader) on account of the changes.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 371:

> 369:   size_t min_threshold = min_free_threshold();
> 370: 
> 371:   if (available < min_threshold) {

In general the changes to this method look somewhat ad-hoc to me. Is the intention to continue using `allocation_headroom` in the future, and this was a "quick-and-dirty" fix? The discrepancy between decision-flow and logging can create confusion. 

I see that the calculated `allocation_headroom` is being used further down (line 448) for control-flow. If possible, I'd define it closer to where it's going to be used, and move the related comments as well so that readers of this code & the comments above will not end up confused.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 374:

> 372:     log_info(gc)("Trigger (%s): Free (" SIZE_FORMAT "%s) is below minimum threshold (" SIZE_FORMAT "%s)",
> 373:                  _generation->name(),
> 374:                  byte_size_in_proper_unit(allocation_headroom), proper_unit_for_byte_size(allocation_headroom),

shouldn't this be `available` instead of `allocation_headroom` in the logs?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 386:

> 384:       log_info(gc)("Trigger (%s): Learning " SIZE_FORMAT " of " SIZE_FORMAT ". Free (" SIZE_FORMAT "%s) is below initial threshold (" SIZE_FORMAT "%s)",
> 385:                    _generation->name(), _gc_times_learned + 1, max_learn,
> 386:                    byte_size_in_proper_unit(allocation_headroom), proper_unit_for_byte_size(allocation_headroom),

same.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 432:

> 430: 
> 431:   double avg_cycle_time = _gc_cycle_time_history->davg() + (_margin_of_error_sd * _gc_cycle_time_history->dsd());
> 432: 

Here again, we seem to have ended up with a discrepancy between the comments and the control flow that those comments applied to, but which has now disappeared? I'd remove the comments in case they are no longer applicable. If the idea is to revisit this in the future, might it be better to capture in a ticket rather than leaving in obsolete comments? In any case, it might make sense to go over these comments and make sure they reflect current reality and clearly call out future plans.

-------------

PR: https://git.openjdk.org/shenandoah/pull/220