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

[Aleksey Shipilev]

On Mon, 9 Sep 2024 19:36:26 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Aleksey Shipilev has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Replace divisions with power-of-two math
>>  - Renames, adjustments after review
>
> src/hotspot/share/cds/archiveHeapWriter.cpp line 185:
> 
>> 183: }
>> 184: 
>> 185: objArrayOop ArchiveHeapWriter::manifest_root_segment(size_t offset, int element_count) {
> 
> The meaning of `manifest` is not very clear to me, and I can't find `manifest` used in this sense in any HotSpot header files. Maybe use `allocate_root_segment` instead?

Yeah, I wanted to avoid `allocate` to avoid the implication we are actually entering a proper allocation path. We are "just" reinterpreting the piece of buffer code as the oop here. But I am not insisting, so I renamed back to `allocate_root_segment`.

> src/hotspot/share/cds/archiveHeapWriter.cpp line 242:
> 
>> 240:     objArrayOop seg_oop = manifest_root_segment(oop_offset, size_elems);
>> 241:     for (int i = 0; i < size_elems; i++) {
>> 242:       root_segment_at_put(seg_oop, i, roots->at(seg_start + i));
> 
> `HeapRootSegments::roots_offset()` is used only here, so I think it's better to avoid having an extra function that you have to understand by reading both of it definition and it usage here. Maybe this can be simplified with:
> 
> 
> int root_index = 0;
> for (size_t seg_idx = 0; seg_idx < segments.count(); seg_idx++) {
>    ...
>     for (int i = 0; i < size_elems; i++) {
>       root_segment_at_put(seg_oop, i, roots->at(root_index++));
>  ```

Right, that is cleaner. Would do this in new commit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1751470779
PR Review Comment: https://git.openjdk.org/jdk/pull/20858#discussion_r1751471143