RFR: 8373819: GenShen: Requested generation may be null [v4]

[William Kemper]

On Fri, 9 Jan 2026 23:05:52 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp line 708:
>> 
>>> 706: 
>>> 707: void ShenandoahGenerationalControlThread::notify_control_thread(MonitorLocker& ml, GCCause::Cause cause, ShenandoahGeneration* generation) {
>>> 708:   assert(_control_lock.is_locked(), "Request lock must be held here");
>> 
>> What is MonitorLocker, and what is `_control_lock`? Why are we checking `_control_lock` here? If ml is being passed here for the purposes of notification on it, it must be the case that it's locked and the assert at line 708 is a leakage of abstraction? I see that your problem here is that along one path you do nothing and and don't post a notification and along another you do some work and post a notification. It sounds like what you want instead is an API of the following shape:
>> 
>> 
>>    bool should_notify_control_thread(cause, generation) { ... };
>> 
>> 
>> and callers might do:
>> 
>> 
>>    MonitorLocker ml(...);
>>    if (should_notify_control_thread(cause, generation)) {
>>       ml.notify();
>>    }
>>    ml.wait();
>
> In addition, all of the various flavours of `notify_control_thread()` with optional parameters should  ideally call down into the version that has all of the parameters specified. In the cases where these parameters aren't specified, the version of the method fills default parameters.
> 
> The fully-parameterized version of the method then contains and consolidates the entire logic and any invariant/assertion checking of the various parameters that make sense, and executes the relevant logic to return a boolean result to allow the caller to notify and/or wait on the monitor.

> What is MonitorLocker, and what is _control_lock?

>From the declaration of `_control_lock`:

  // This lock is used to coordinate setting the _requested_gc_cause, _requested generation
  // and _gc_mode. It is important that these be changed together and have a consistent view.
  Monitor _control_lock;


There are a few different paths into `notify_control_thread`. Rather than expose the locking protocol to callers and duplicating the logic to notify or not, we have an entry point that acquires the lock for the caller before calling down into the method that does the actual work. On some paths, the lock is already held for other reasons, so the overload is provided for these cases (the lock is not reentrant).

> optional parameters should ideally call down into the version that has all of the parameters specified.

This is what is happening, except in the case of allocation failures where the caller does not "know" the generation being collected (assuming a collection is even running). The implementation here intentionally hides as much information as possible. Some callers do not need to provide the generation. In some cases, only the control thread itself can provide the correct generation. We could expose this piece of data to other threads so that could always include a generation in their request, but the control thread would still end up ignoring it. This would also be confusing on a first read of the code.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28932#discussion_r2683457097