RFR: 8293623: Simplify G1ConcurrentRefineThreadControl [v5]

[Kim Barrett]

On Fri, 15 Dec 2023 14:28:51 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:
> 
>   remove unnecessary code

The vector's size should be used to track how many threads have been created.
Detailed comments show what I mean by this.

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

> 66: G1ConcurrentRefineThreadControl::~G1ConcurrentRefineThreadControl() {
> 67:   for (uint i = 0; i < max_num_threads(); i++) {
> 68:     G1ConcurrentRefineThread* t = _threads.at(i);

With size tracking, this whole definition can be replaced with

  while (!_threads.is_empty()) {
    delete _threads.pop();
  }

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

> 85: 
> 86:   if (max_num_threads() > 0) {
> 87:     _threads.append(create_refinement_thread(0, true));

push is more natural than append here, since the result is not needed.

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

> 93:     if (UseDynamicNumberOfGCThreads) {
> 94:       for (uint i = 1; i < max_num_threads(); ++i) {
> 95:         _threads.append(nullptr);

This is not needed with size-based tracking.

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

> 97:     } else {
> 98:       for (uint i = 1; i < max_num_threads(); ++i) {
> 99:         _threads.append(create_refinement_thread(i, true));

Again here, push is more natural than append.

With size-based tracking, better would be to add a new private helper function like this:

  bool ensure_threads_created(uint worker_id, bool initializing) {
    assert(worker_id < max_num_threads(), "precondition");
    while ((uint)_threads.size() <= worker_id) {
      _threads.push(create_refinement_thread(_threads.size(), initializing);
      if (_threads.last() == nullptr) {
        _threads.pop();
        return false;
      }
    }
    return true;
  }

and replace the loop here with

  if (!ensure_threads_created(max_num_threads() - 1, true)) {
    vm_shutdown_during_initialization("Could not allocate refinement threads");
    return JNI_ENOMEM;
  }

and also use the new helper in `activate`.

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

> 123:       return false;
> 124:     }
> 125:     _threads.at(worker_id) = thread_to_activate;

With size-based tracking and the suggested helper function, replace this whole function with

  if (ensure_threads_created(worker_id, false)) {
    _threads.at(worker_id)->activate();
    return true;
  } else {
    return false;
  }

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17077#pullrequestreview-1785048657
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428578699
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428579029
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428580007
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428581231
PR Review Comment: https://git.openjdk.org/jdk/pull/17077#discussion_r1428599201