RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v9]

[Albert Mingkun Yang]

On Fri, 3 Nov 2023 13:53:35 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1YoungCollector.cpp line 87:
>> 
>>> 85:              GCCause::to_string(_pause_cause),
>>> 86:              _collector->evacuation_pinned() ? " (Pinned)" : "",
>>> 87:              _collector->evacuation_alloc_failed() ? " (Allocation Failure)" : "");
>> 
>>> GC(6) Pause Young (Normal) (Pinned) (Evacuation Failure)
>> 
>> I wonder if the last two can be merged into one `()`, sth like `(Pinned / ...)`, because they are on the same abstraction level.
>
> Parsing the separate components is easier :) Not sure if these tags in any way ever indicated some level of abstraction.
> 
> I do not have a strong opinion here. The combinations
> 
> (Pinned)
> (Allocation Failure)
> (Pinned + Allocation Failure)  // or the other way around, or some other symbol for "+" or no symbol at all?
> 
> are fine with me (and I thought about doing something more elaborate here), but my concern has been that any complicated string makes it less unique (e.g. `(Allocation Failure)` vs. "Allocation Failure") and adds code both to implement and parse the result.
> 
> Much more disrupting is likely that there is no "Evacuation Failure" string any more. But log messages are not part of the external interface, and we should not want to change them just because.

The example looks good to me.

>> src/hotspot/share/gc/g1/g1_globals.hpp line 327:
>> 
>>> 325:           range(1, 256)                                                     \
>>> 326:                                                                             \
>>> 327:   product(uint, G1NumCollectionsKeepPinned, 8, DIAGNOSTIC,                  \
>> 
>> Any particular reason this is not `EXPERIMENTAL`?
>
> Changing this does not in any way enable risky/experimental code not fit for production. This knob is for helping diagnose performance issues.
> 
> G1 does have its fair share of experimental options, but all/most of these were from the initial import where G1 as a whole had been experimental (unstable) for some time.

This flag conceptually related (or similar) to `G1RetainRegionLiveThresholdPercent`, which is an exp, so I thought they should be the same category.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381748512
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381747902