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

[Kim Barrett]

On Thu, 22 Sep 2022 15:07:04 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Kim Barrett has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - lengthen some names
>>  - improve comments for primary do_refinement_step
>>  - add is_pending_cards_target_initialized
>
> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 203:
> 
>> 201: }
>> 202: 
>> 203: void G1ConcurrentRefine::update_pending_cards_target(double logged_cards_scan_time_ms,
> 
> `logged_cards_scan_time_ms` includes time for merging card (i think it should include time for concatenation),  probably better to name it as `logged_cards_processing_time_ms` as in the caller. Additionally, we use `processed_logged_cards` and not `scanned_logged_cards`

Agreed that "scan" isn't right.  I dropped that word in several places.  Just
dropped rather than replacing with "processing" because of excessive length
and I didn't think it added anything.

> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 246:
> 
>> 244:     // drastically, record that adjustment is needed and kick the primary
>> 245:     // thread, in case it is waiting.
>> 246:     _dcqs.set_max_cards(SIZE_MAX);
> 
> Don't we already set `_dcqs.set_max_cards(SIZE_MAX)` in `G1DirtyCardQueueSet::concatenate_logs()`

We do, but I don't want to rely on that far away and done for other reasons
code. The reason for setting in concatenate_logs is to cut off any refinement
by handle_completed_buffer (via flushing buffers). It isn't entirely necessary
for that either, as there are other reasons why that function won't do any
refinement. I waffled about whether to keep or remove it.

> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 258:
> 
>> 256: // This reduces the number of primary thread wakeups that just immediately
>> 257: // go back to waiting, while still being responsive to behavior changes.
>> 258: static uint64_t compute_adjust_delay(double available_ms) {
> 
> `delay` and `wait` used interchangeably, maybe better to choose one

s/delay/wait_time_ms/

> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 116:
> 
>> 114:   Ticks _last_deactivate;
>> 115:   bool _needs_adjust;
>> 116:   G1ConcurrentRefineThreadsNeeded _threads_needed;
> 
> `_threads_wanted` vs. `_threads_needed` makes for difficult reading in many places

The intent is that "needed" refers to the number required to reach the goal,
while "wanted" is the number the controller wants active.  "wanted" may be
less than "needed" for a variety of reasons.

There were a couple of places where that intended nomenclature wasn't
followed, which I've fixed.

There is still G1ConcurrentRefine::_threads_wanted (the number of active
threads wanted by the controller) and G1ConcurrentRefine::_threads_needed (the
object that provides the calculation of the needed threads). From usage I
hope there isn't confusion, but I'm open to suggestions for better names.

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

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