RFR: 8338912: CDS: Segmented roots array

[Ioi Lam]

On Wed, 4 Sep 2024 18:44:37 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`

I think the design is good. I have some suggestions for readability and simplification.

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

> 210:   int segment_beg = 0;
> 211:   while (segment_beg < length) {
> 212:     // Guess the size of next segment.

nit: it's not a guess when you can't be wrong. I think this comment is not needed.

Also, can you use `HeapRoots::length_for_segment()` here?

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

> 215: 
> 216:     size_t oop_offset = _buffer_used;
> 217:     _buffer_used = oop_offset + bytes_size;

Assert that `_buffer_used` is region aligned.
Assert that `bytes_size` is the same as MIN_GC_REGION_ALIGNMENT

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

> 217:     _buffer_used = oop_offset + bytes_size;
> 218:     ensure_buffer_space(_buffer_used);
> 219: 

There's a lot of low-level calculation here that's hard to understand. I would suggest splitting this "while" body into smaller functions, something like: 


while (segment_beg < length) {
    int elem_count = MIN2(max_elem_count, length - segment_beg);
    objArrayOop segment_oop = allocate_root_segment(elem_count);
    for (int i = 0; i < elem_count; i++) {
        oop val = roots->at(segment_beg + i);
        root_segment_at_put(segment_oop, i, val);
    }
}


BTW, I think you can remove the special relocation code in `ArchiveHeapWriter::relocate_embedded_oops` by storing the requested address of `val` into `segment_oop`. You also need to call `mark_oop_pointer()` on each slot.

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

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

Since we have 2 levels of objArrays, I think it's better to name them as

- root_segments[] -> root_segment0, root_segment1,  ...



objArrayOop HeapShared::root_segment(int segment_idx) {

  objArrayOop segment = (objArrayOop)_root_segments->at(segment_idx).resolve();
  return segment;

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

PR Review: https://git.openjdk.org/jdk/pull/20858#pullrequestreview-2281856065
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1744806024
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1744829837
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1744821170
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1744819019