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