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

[Thomas Schatzl]

On Fri, 30 Oct 2020 02:17:03 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 updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into jmap-par
>  - Merge remote-tracking branch 'upstream/master' into jmap-par
>  - update the return type of iterable_blocks to size_t
>  - cast iterable_blocks to int in claim_and_get_block()
>  - fix constant coding style and do code refine
>  - Refine HeapBlockClaimer implementation
>  - 8252103: Parallel heap inspection for ParallelScavengeHeap
>    
>    - Parallel heap iteration support for PSS
>    - JBS:  https://bugs.openjdk.java.net/browse/JDK-8252103

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

> 291: };
> 292: 
> 293: // The HeapBlockClaimer is used during parallel iteration over heap,

s/over heap/over the heap/

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

> 302:   // Claim the block and get the block index.
> 303:   bool claim_and_get_block(int* block_index);
> 304:   static const int EdenIndex = -2;

Please explain what the values of `_claimed_index` mean if they are >= 0 (i.e. parts of old gen).

Also add a newline before the static consts which I personally prefer to locate before methods if possible.

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

> 559:   assert(block_index != NULL, "Invalid index pointer");
> 560:   *block_index = Atomic::fetch_and_add(&_claimed_index, 1);
> 561:   int itrable_blocks = (int)ParallelScavengeHeap::heap()->old_gen()->iterable_blocks();

s/itrable/iterable

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

> 551:       young_gen()->to_space()->object_iterate(cl);
> 552:     } else {
> 553:       old_gen()->block_iterate(cl, block_index);

here is a type mismatch between the type of `block_index` and the second parameter (int vs. uint) too

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

> 211:     }
> 212:     assert(begin <= start && start < end,
> 213:            "object %p must in the range of [%p, %p)\n", start, begin, end);

use [I]PTR_FORMAT instead of %p.
The assert is too strict too. The addition in line 210 can make start beyond end.

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

> 189:  */
> 190: void PSOldGen::block_iterate(ObjectClosure* cl, uint block_index) {
> 191:   MutableSpace *space = object_space();

Similar to `object_iterate()` this method should be implemented in the underlying `MutableSpace`, not in `PSOldGen` and just forward to the `MutableSpace` here. While the current design may be debatable, we should adhere to it in new code.

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

> 297: // The old spaces are divided into serveral fixed-size blocks.
> 298: class HeapBlockClaimer : public StackObj {
> 299:   int _claimed_index;

There is severe type confusion throughout the code about this. Sometimes the code implicitly casts to uint, sometimes relying on automatic widening to do the right thing.

Also, please use a larger type on 64 bits as the range of int plus the block size can not span the whole 64 bits, only 52 bits. While "large", there seems no reason to simply use something like `ssize_t` instead (if signedness is required).

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

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