RFR: 8293623: Simplify G1ConcurrentRefineThreadControl [v8]

[Kim Barrett]

On Sat, 16 Dec 2023 05:31:09 GMT, Lei Zaakjyu <duke at openjdk.org> wrote:

>> 8293623: Simplify G1ConcurrentRefineThreadControl
>
> Lei Zaakjyu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 77:

> 75:   while ((uint)_threads.length() <= worker_id) {
> 76:     G1ConcurrentRefineThread* rt = create_refinement_thread(_threads.length(), initializing);
> 77:     _threads.push(rt);

If capturing the create result in a variable, do the null check _before_ pushing.

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 101:

> 99:     if (UseDynamicNumberOfGCThreads) {
> 100:       for (uint i = 1; i < max_num_threads(); ++i) {
> 101:         _threads.push(nullptr);

Now that size/length is being used to track how many threads are in _threads, this is incorrect.  This fills
_threads with nulls, with length == max_num_threads at the end.  So later calls to ensure_threads_created
will do nothing.  I'm surprised this doesn't lead to crashes when attempting to activate threads other than
the primary thread.

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 140:

> 138:   for (uint i = 0; i < max_num_threads(); i++) {
> 139:     if (_threads.at(i) != nullptr) {
> 140:       _threads.at(i)->stop();

If iteration is bounded by length rather than max_num_threads then there's no need for the null check.

src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 76:

> 74: 
> 75: private:
> 76:   bool ensure_threads_created(uint worker_id, bool initializing);

More typical style would be to place this in the initial private block, like after create_refinement_thread,
rather than in a new private block.

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

PR Review: https://git.openjdk.org/jdk/pull/17077#pullrequestreview-1785167597
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428785485
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428786593
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428786841
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428784801