RFR: 8357218: G1: Remove loop in G1CollectedHeap::try_collect_fullgc [v4]

[Kim Barrett]

On Tue, 20 May 2025 07:04:34 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> How about introducing a local var to better reflect the semantics?
>> 
>> 
>> // GCs always run-to-completion once prologue succeeds.
>> bool is_gc_succeeded = op.prologue_succeeded();
>> return is_gc_succeeded;
>> 
>> 
>> This way we don't need to maintain a "redundant" field in `VM_G1CollectFull`.
>> 
>> (Thomas also asked whether the same field in `VM_G1CollectForAllocation` is actually needed -- seems that `_gc_succeeded == prologue_succeeded()` holds as well, so we can potential remove that one as well.)
>> 
>> If you dislike the suggestion, I will add back the removed field.
>
> The field does not need to be re-added in the full gc VM op, just add an accessor `gc_succeeded()` that returns the internal `prologue_succeeded()`.
> 
> I will file a bug for removing the `_gc_succeeded` field in `VM_G1CollectForAllocation`, because it is always true if `doit` ran.

I think the question that clients care about is whether the gc succeeded.
Whether the prologue succeeded or not might be used as part of the
implementation of that. That is, I think gc_succeeded ought to be part of the
public API for GC operations, and prologue_succeeded shouldn't be, with some
gc_succeeded operations being implemented as just a call to
prologue_succeeded.

That probably shouldn't be part of this change though. Instead, I'd prefer
this change maintain the status quo in this area. Or do what @tschatzl
suggested, which I saw as I was about to post this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25296#discussion_r2097153112