RFR: 8137022: Concurrent refinement thread adjustment and (de-)activation suboptimal [v3]

[Kim Barrett]

On Tue, 27 Sep 2022 12:54:30 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Kim Barrett has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - wanted vs needed nomenclature
>>  - remove several spurious "scan"
>>  - delay => wait_time_ms
>
> src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 61:
> 
>> 59:     SuspendibleThreadSetJoiner sts_join;
>> 60:     G1ConcurrentRefineStats active_stats_start = _refinement_stats;
>> 61:     report_active("Activated");
> 
> Because `G1ConcurrentRefineThread::run_service()` is shared between the primary and secondary thread there will be an "Activated" log message for each periodic wake of the primary refinement thread. This is on the trace level, but I think we should only call `report_active()` when we know there is work to be done by the thread, not just checks.

The point of this message is mostly to see when these threads are
(potentially) competing with application threads for processor time, which
occurs regardless of whether they do anything or just quickly go back to
waiting.

At this point the primary thread doesn't know whether there is work to be
done, since it hasn't (successfully) called update_threads_periodically yet.
(It can't call that until inside the STS context, since it isn't thread-safe
against a GC. It can't usefully call it between the STS entry and the report,
since it might not succeed and need to be retried, which we deal with by
trying to do some work before trying again, instead of useless spinning.)
The same is true when a thread is resumed after a safepoint (even more so
there, since a secondary thread might already be unwanted).

Conditionally reporting activation would need to be mirrored when reporting
deactivation.

For all those reasons, I think it should be left as is.

> src/hotspot/share/gc/g1/g1Policy.cpp line 963:
> 
>> 961:             hcc_cards,
>> 962:             merge_hcc_time_ms,
>> 963:             (exceeded_goal ? " (exceeded goal)" : ""));
> 
> Since the pending cards target is initialized to `SIZE_MAX`, the first GC this will always print: 
> 
> GC(0) GC refinement: goal: 18446744073709551615 ...
> 
> 
> Not sure if printing 0 is much better but it looks more "buggy" to print `SIZE_MAX` imo.

Oh, I forgot about that.  I was going to try to come up with something better there, like somehow printing "unlimited" or something like that.

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

PR: https://git.openjdk.org/jdk/pull/10256