RFR: 8373253: Re-work InjectGCWorkerCreationFailure for future changes [v3]

[Stefan Johansson]

On Wed, 7 Jan 2026 12:52:25 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

>> This PR slightly changes when we may `InjectGCWorkerCreationFailure`.  At the moment we wait until `is_init_completed()`. I am changing this to instead be after `_created_workers > 0`. The reason is that we might in the future, https://bugs.openjdk.org/browse/JDK-8367993,  create even more threads "on demand", and if so, we would fail VM creation if we inject worker creation failures after the VM is initiated but before we have created any of the worker threads.
>> 
>> This change should not change the current behaviour. But help future improvements.
>> 
>> I have tested this on `test/hotspot/jtreg/gc/stress/gcold/TestGCOldWithG1.java`, the only place where this flag is tested to my knowledge.
>
> Leo Korinth has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into _8373253
>  - Fixup after comment from Ivan.
>  - 8373253: Re-work InjectGCWorkerCreationFailure for future changes

Have an alternative solution, see below.

One difference is that with this solution the initialization of a work-gang after `is_init_completed()` starts returning true, will only get one worker before the injected creation failures start occurring. This is only a difference is `UseDynamicNumberOfGCThreads` is disabled and I don't see that as a problem for a testing feature.

src/hotspot/share/gc/shared/workerThread.cpp line 123:

> 121:     return _active_workers;
> 122:   }
> 123: 

I find adding this logic to `set_active_workers(...)` a bit odd. I see that it will have the desired effect, but the flag is about creation failure and I think keeping the logic in `create_worker(...)` would be better.

For the upcoming change to lazy initialize concurrent mark we need a way to make sure at least one worker is created for each work-gang. This is why we add the above check on `_current_workers`, but this check could also be added to the original code path, as an additional requirement similar to `is_init_completed()`.

My preferred solution would be to revert this change to `set_active_workers(...)` and instead update the check in `create_worker(...)`. To make the code even clearer we could separate the conditions into a helper like:


if (InjectGCWorkerCreationFailure && allow_creation_failure()) {
  return nullptr;
}

Where the helper is implemented something like this:

bool WorkerThreads::allow_creation_failure() const {
  if (!is_init_completed()) {
    // Never allow creation failures during VM init
    return false;
  }

  if (_created_workers == 0) {
    // Never allow creation failures of the first worker, it will cause the VM to exit
    return false;
  }

  return true;
}

I've discussed this a bit with Leo and Stefan K offline and I think they more or less agree with this proposal.

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

Changes requested by sjohanss (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28719#pullrequestreview-3649858223
PR Review Comment: https://git.openjdk.org/jdk/pull/28719#discussion_r2681471572