On Mon, 19 Oct 2020 13:09:34 GMT, Lin Zang <lz...@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

Reply via email to