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

[Kim Barrett]

On Tue, 27 Sep 2022 11:33:05 GMT, Thomas Schatzl <tschatzl 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/g1ConcurrentRefine.cpp line 77:
> 
>> 75:       } else {
>> 76:         delete t;
>> 77:       }
> 
> Not sure this early-bail out is necessary; C++ seems to define `nullptr` values passed to `delete` well enough (https://en.cppreference.com/w/cpp/language/delete) to skip this complication.
> Feel free to ignore.

The assert that nothing strange has happened is part of the point here.  And there could be lots of thread entries on a sufficiently large machine.

> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 219:
> 
>> 217:   // Deduct predicted cards in thread buffers to get target.
>> 218:   size_t new_target = budget - MIN2(budget, predicted_thread_buffer_cards);
>> 219:   // Add some hysterisis with previous values.
> 
> Suggestion:
> 
>   // Add some hysteresis with previous values.

Done.

> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 233:
> 
>> 231:                                          size_t predicted_thread_buffer_cards,
>> 232:                                          double goal_ms) {
>> 233:   if (!G1UseConcRefinement) return;
> 
> I would prefer either braces or an additional newline after this statement. When initially reading this I thought there were some indentation error.

Done.

> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 198:
> 
>> 196:   void reduce_threads_wanted();
>> 197: 
>> 198:   // Test whethre the thread designated by worker_id should be active.
> 
> s/whethre/whether

Done.

> src/hotspot/share/gc/g1/g1ConcurrentRefineThreadsNeeded.cpp line 44:
> 
>> 42: // Estimate how many concurrent refinement threads we need to run to achieve
>> 43: // the target number of card by the time the next GC happens.  There are
>> 44: // several additional desirements we'd like to achieve while meeting that
> 
> According to https://en.wiktionary.org/wiki/desirement this is defined as
> 
>   Something that is [desired](https://en.wiktionary.org/wiki/desired), but not absolutely required.
> 
> (This word was new to me).
> 
> I think that "desirements" already includes the phrase "we'd like to achieve". I.e. something that is desired, but is not absolutely required is already something "we'd like to achieve".
> 
> So I would like to suggest to either remove the phrase "we'd like to achieve" or reformulate the sentence as "... several additional/secondary goals we would like to achieve while meeting that (main) goal".

Done.  Using "secondary goals".

> src/hotspot/share/gc/g1/g1ConcurrentRefineThreadsNeeded.cpp line 79:
> 
>> 77:   // Estimate number of cards that need to be processed before next GC.  There
>> 78:   // are no incoming cards when time is short, because the controller activates
>> 79:   // refinement by mutator threads when there to a GC, to stay on target even
> 
> "refinement by mutator threads when there to a GC" I do not understand this part of the sentence, what does "there to a gc" mean?
> 
> Maybe this should means:
> 
> "..., because in this case the controller activates refinement by mutator threads to stay on target even..."

Done.  Something like that was intended.  Not sure what happened, maybe I got interrupted in the middle of editing.

> src/hotspot/share/gc/g1/g1ConcurrentRefineThreadsNeeded.cpp line 116:
> 
>> 114:   // excess cards to process.  Just one thread might not be sufficient, but
>> 115:   // we don't have any idea how many we actually need.  Eventually the
>> 116:   // prediction machinary will warm up and we'll be able to get estimates.
> 
> s/machinary/machinery

Done.

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

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