RFR: 8350621: Code cache stops scheduling GC [v2]

[Thomas Schatzl]

On Thu, 10 Jul 2025 20:53:17 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   * kbarret review:
>>     - restructure code in `try_collect_concurrently`
>>     - fix asserts - they only worked in the test because of whitebox being active
>>     - fix comments
>>   * threalmdoerr review:
>>     - fix test to be more lenient
>
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1755:
> 
>> 1753:         old_marking_started_before = old_marking_started_after;
>> 1754:       }
>> 1755:     } else if (GCCause::is_codecache_requested_gc(cause) && op.marking_in_progress()) {
> 
> I wonder if this might be simpler if the codecache-request case were
> completely handled by it's own clause. This would involve "duplicating" some
> of the user-request case. But I think that could be done by chunking that into
> a couple helper functions, bounded by the existing assertions in the
> user-request case. So something like this (I don't love the names of the
> helpers here):
> 
> if (is_codecache_requested_gc(cause)) {
>   if (op.marking_in_progress()) {
>     LOG... ;
>     return true;
>   }
>   // is there some assert similar to the user-request assert that makes sense here?
>   if (wait_for_completion(old_marking_started_before,
>                           old_marking_started_after,
>                           old_marking_completed_after)) {
>     return true;
>   }
>   // is there some assert similar to the user-request assert that makes sense here?
>   if (need_normal_retry(op)) {
>     continue;
>   }
> }
> 
> And similarly in the user-request case.  And obviously with more comments :)
> 
> While looking at that, I noticed that the whitebox control stall in the
> user-request case is doing exactly what the comment about it says not to do.
> It is waiting for control to be released, while the comment says not to do
> that. This of course means there could be multiple complete collections (and
> not just STW full collections) before it can proceed. This seems to be in the
> initial published webrev for JDK-8240239. The immediately preceding (draft,
> non-public) webrev has that comment but different code, so it looks like the
> code was changed but the comment wasn't updated. Bad Kim!
> 
> I will file a bug for that and assign it to myself. If you decide to adopt
> something like the above suggested refactoring, just retain that code and
> comment as-is. I'll see if I can recall what happened there, and update the
> comment accordingly. Unfortunately, the (non-public) email discussion between
> those versions doesn't make any mention of this change.

I did the suggested change, the names probably aren't that much better.
Retested tier1-5.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26189#discussion_r2209668371