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

[Kim Barrett]

On Wed, 21 Sep 2022 07:43:08 GMT, Stefan Johansson <sjohanss 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 267:
> 
>> 265:     // If target is "unbounded" then wait forever (until explicitly
>> 266:     // activated).  This happens during startup, when we don't bother with
>> 267:     // refinement.
> 
> We use `SIZE_MAX` to signal that the target is "unbounded", like the comment says. We do this comparison a few times and I think it would make sense to define a constant with a more descriptive name for this.

Added `is_pending_cards_target_initialize` and `PendingCardsTargetUninitialized`.

> src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 174:
> 
>> 172:     if (!try_refinement_step(cr()->pending_cards_target())) {
>> 173:       // Proceed with fewer threads if target reached.
>> 174:       cr()->reduce_threads_wanted();
> 
> Not sure I see the reason to never try refinement when adjusted. I get that there is a case where we don't need to do refinement but that seems to be handled by an "early return" in `try_refinement_step(...)`. 
> 
> If we really want to skip refinement I would prefer an early return here as well. 
> 
> if (cr()->adjust_threads_periodically()) {
>   // If adjustment done, don't do any refinement this round, since this thread
>   // might no longer be wanted active.
>   return;
> }
> ...

I expanded the comments here to give more explanation and justification.

> src/hotspot/share/gc/g1/g1ConcurrentRefineThreadsNeeded.hpp line 40:
> 
>> 38:   G1Policy* _policy;
>> 39:   double _update_period_ms;
>> 40:   double _predicted_time_ms;
> 
> I like to keep the names short but I wonder if it would make sense to include until gc here:  `_predicted_time_until_gc_ms`

That seems reasonable.
`predicted_time_ms` => `predicted_time_until_next_gc_ms`
`predicted_cards` => `predicted_cards_at_next_gc`

> src/hotspot/share/gc/g1/g1Policy.cpp line 1124:
> 
>> 1122:   uint remaining = target - MIN2(target, full);
>> 1123:   size_t bytes = remaining * HeapRegion::GrainBytes;
>> 1124:   return bytes - MIN2(bytes, allocator->used_in_alloc_regions());
> 
> From what I can tell it's only this last line that require the `Heap_lock` to be held, so we could still do an estimate (slightly worse) even if we fail to take the lock. So my question is would it make sense to move the `Heap_lock->try_lock()` in here and just skip including `allocator->used_in_alloc_regions()` in the calculation when not getting the lock?

`_g1h->young_list_count()` and `young_list_target_length()` can also be
changed, protected by `Heap_lock`.

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

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