On Fri, 3 Nov 2023 13:53:35 GMT, Thomas Schatzl <tscha...@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
