RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]

[Stefan Johansson]

On Mon, 19 Oct 2020 13:09:34 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> - Parallel heap iteration support for PSS
>> - JBS:  https://bugs.openjdk.java.net/browse/JDK-8252103
>
> Lin Zang has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR.

Hi Lin,

A nice improvement over the last version. I still have some comments though.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 543:

> 541: 
> 542: void ParallelScavengeHeap::object_iterate_parallel(ObjectClosure* cl,
> 543:                                                    uint worker_id,

`worker_id` is never used, please remove.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 562:

> 560: HeapBlockClaimer::HeapBlockClaimer(uint n_workers) :
> 561:     _n_workers(n_workers), _n_blocks(0), _claims(NULL) {
> 562:   assert(n_workers > 0, "Need at least one worker.");

Unless I'm missing something the only use of `_n_workers` is the assert here and I think it's better to just skip it.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 565:

> 563:   size_t old_gen_used = ParallelScavengeHeap::heap()->old_gen()->used_in_bytes();
> 564:   size_t block_size = ParallelScavengeHeap::heap()->old_gen()->iterate_block_size();
> 565:   uint n_blocks_in_old = old_gen_used / block_size + 1;

Instead of doing this calculation here, what do you think about making `iterate_block_size()` a constant in `PSOldGen` and instead adding a function that returns the number of blocks available, something like:
`iterable_blocks()`

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 591:

> 589:     }
> 590:     next_index = Atomic::load(&_unclaimed_index);
> 591:   }

I think this can be simplified a bit. If you use Atomic::fetch_and_add() for the claiming, you only need `_unclaimed_index` and can get rid of `_claims`. I think you might want to rename it to `_claimed_index` or something. It could look something like this:
Suggestion:

  assert(block_index != NULL, "Invalid index pointer");
  *block_index = Atomic::fetch_and_add(&_claimed_index, 1);
  if (*block_index  >= _n_blocks) {
    return false;
  }
  return true;

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 604:

> 602:       _thread_num(thread_num),
> 603:       _heap(ParallelScavengeHeap::heap()),
> 604:       _claimer(thread_num == 0 ? ParallelScavengeHeap::heap()->workers().active_workers() : thread_num) {}

As mentioned, the `_claimer` don't need the number of threads so it can be removed from here as well.

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp line 297:

> 295: // The HeapBlockClaimer is used during parallel iteration over heap,
> 296: // allowing workers to claim heap blocks, gaining exclusive rights to these blocks.
> 297: // The eden, survivor spaces are treated as single blocks as it is hard to divide

Suggestion:

// The eden and survivor spaces are treated as single blocks as it is hard to divide

src/hotspot/share/gc/parallel/psOldGen.cpp line 200:

> 198:   // iterate objects in block.
> 199:   HeapWord* end = MIN2(top, begin + _iterate_block_size);
> 200:   // There can be no object between begin and end.

I would change this to something like:
"Only iterate if there are objects between begin and end.

src/hotspot/share/gc/parallel/psOldGen.hpp line 56:

> 54: 
> 55:   // Block size for parallel iteration
> 56:   const size_t _iterate_block_size;

Turn into constant.

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

PR: https://git.openjdk.java.net/jdk/pull/25