RFR: 8338912: CDS: Segmented roots array [v4]

[Ioi Lam]

On Fri, 6 Sep 2024 17:06:16 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Attempt to drop the min region alignment with [JDK-8337828](https://bugs.openjdk.org/browse/JDK-8337828) highlights an interesting trouble. The roots array we are creating during the dump time can easily be larger than the min region alignment. We are currently "lucky" none of our tests hit this limit. AFAICS, about 128K classes would be enough to hit the current 1M min region alignment. Dropping the min region alignment to 256K starts to fail the test with "only" 30K classes, [JDK-8338856](https://bugs.openjdk.org/browse/JDK-8338856).
>> 
>> We can slice that heap root array, and thus untie the roots count from the min region alignment. I am submitting something that works, but this might not be the final form for it. I would like @iklam to poke holes in this approach :)
>> 
>> Additional testing:
>>  - [x] macos-aarch64-server-fastdebug, `runtime/cds`
>>  - [ ] linux-aarch64-server-fastdebug, `all`
>>  - [ ] linux-x86_64-server-fastdebug, `all`
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Amend printout as well

Looks good. Just some nits on naming, etc.

src/hotspot/share/cds/archiveHeapLoader.cpp line 381:

> 379:     int max_size = roots.segment_max_size_bytes();
> 380:     HeapShared::init_root_segment_max_size(max_size);
> 381:     intptr_t base_roots_oop = bottom + roots.base_offset();

Since people are more likely to read this function (using the roots) than archiveHeapWriter.cpp (writing the roots), I think it's better to add a comment here. `base_roots_oop` should also be renamed for clarity.


// The heap roots are stored in one or more segments that are laid out consecutively.
// The byte size of each segment (except for the last one) is max_size.
intptr_t first_segment_addr = bottom + roots.base_offset();

src/hotspot/share/cds/archiveHeapWriter.cpp line 223:

> 221: 
> 222:   HeapRoots heap_roots(_buffer_used,
> 223:                        roots->length(),

We use `GrowableArrayCHeap<oop, mtClassShared>* roots` in a few places in this file, and having another variable `heap_roots` makes it difficult to distinguish between the two.

Maybe use this instead? I think local variables can be called `segments` instead of `heap_root_segments` when it's obvious.


HeapRootSegments segments(...)

src/hotspot/share/cds/archiveHeapWriter.cpp line 629:

> 627:     if (UseCompressedOops) {
> 628:       for (int i = 0; i < length; i++) {
> 629:         narrowOop* addr = (narrowOop*)(buffered_obj + requested_obj->obj_at_offset<narrowOop>(i));

I know this matches the old code, but I think it will be easier to read if we use this:


narrowOop* addr = (narrowOop*)(buffered_obj + buffered_obj->obj_at_offset<narrowOop>(i));

src/hotspot/share/cds/heapShared.cpp line 239:

> 237:   }
> 238: 
> 239:   objArrayOop roots = (objArrayOop)_root_segments->at(segment_idx).resolve();

Should be `segment = ...`

src/hotspot/share/cds/heapShared.cpp line 775:

> 773: 
> 774: void HeapShared::add_root_segment(oop segment_oop) {
> 775:   if (segment_oop != nullptr) {

null check is not necessary. Also parameter type should be objArray.

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

PR Review: https://git.openjdk.org/jdk/pull/20858#pullrequestreview-2286832237
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1747506715
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1747505297
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1747493513
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1747522640
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1747521760