RFR: 8339668: Parallel: Adopt PartialArrayState to consolidate marking stack in Full GC [v5]

[Zhengyu Gu]

On Wed, 22 Jan 2025 11:11:11 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Zhengyu Gu 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 17 additional commits since the last revision:
>> 
>>  - Yang's comment
>>  - Merge branch 'master' into JDK-8339668
>>  - Adopt latest PartialArrayStats changes
>>  - Merge branch 'master' into JDK-8339668
>>  - Merge branch 'master' into JDK-8339668
>>  - @tschatzl's ScannerTask changes
>>  - @tschatzl's comment
>>  - v8
>>  - v7
>>  - v6
>>  - ... and 7 more: https://git.openjdk.org/jdk/compare/7a117f9d...332c1cdd
>
> src/hotspot/share/gc/parallel/psCompactionManager.cpp line 43:
> 
>> 41: #include "oops/objArrayKlass.inline.hpp"
>> 42: #include "oops/oop.inline.hpp"
>> 43: #include "utilities/checkedCast.hpp"
> 
> Can this be removed now?

Done.

> src/hotspot/share/gc/parallel/psCompactionManager.cpp line 61:
> 
>> 59: ParCompactionManager::ParCompactionManager(PreservedMarks* preserved_marks,
>> 60:                                            ReferenceProcessor* ref_processor)
>> 61:   :_partial_array_splitter(_partial_array_state_manager, ParallelScavengeHeap::heap()->workers().max_workers()),
> 
> I would probably pass `parallel_gc_threads` into the constructor of `ParCompactionManager`, instead of calling `max_workers()` directly here, in order to maintain a single source of truth. YMMV.

Fixed

> src/hotspot/share/gc/parallel/psCompactionManager.cpp line 128:
> 
>> 126:   size_t array_length = obj_array->length();
>> 127:   size_t initial_chunk_size =
>> 128:     // The destination array is unused when processing states.
> 
> This comment is not very helpful; I had to look into the callee to find out it's referring to `nullptr` in the args and why one can use `nullptr` here, but that process is fairly straightforward, so this comment can probably be dropped.

Fixed

> src/hotspot/share/gc/shared/taskqueue.hpp line 566:
> 
>> 564: // Discriminated union over oop/oop*, narrowOop*, and PartialArrayState.
>> 565: // Uses a low tag in the associated pointer to identify the category.
>> 566: // Oop/oop* are overloaded using the same tag because they can not appear at the
> 
> The reason why "they can not appear at the same time" is that one is for young-gc and the other full-gc, right? If so, maybe state that explicitly.

As far as Parallel, `PromotionManager` uses `ScannerTask` to wrap around `*oop` and `CompactionManager` to wrap around `oop`, which is true. But I don't think it has to have such correlation. 

Probably should phase as `ScannerTask` wraps `*oop` or `oop` using the same tag, because they cannot be mixed in the same queue. 

What do you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21089#discussion_r1929611411
PR Review Comment: https://git.openjdk.org/jdk/pull/21089#discussion_r1929611365
PR Review Comment: https://git.openjdk.org/jdk/pull/21089#discussion_r1929611400
PR Review Comment: https://git.openjdk.org/jdk/pull/21089#discussion_r1929611054