RFR: 8251330: Reorder CDS archived heap to speed up relocation [v3]

[Ioi Lam]

On Tue, 27 Feb 2024 18:56:54 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> We should reorder the archived heap to segregate the objects that don't need marking. This will save space in the archive and improve start-up time
>> 
>> This patch reorders the archived heap to segregate the objects that don't need marking. The leading zeros present in the bitmaps thanks to the reordering can be easily removed, and this the size of the archive is reduced.
>> 
>> Given that the bitmaps are word aligned, some leading zeroes will remain. Verified with tier 1-5 tests.
>> 
>> Metrics, courtesy of @iklam :
>> ```calculate_oopmap: objects = 15262 (507904 bytes, 332752 bytes don't need marking), embedded oops = 8408, nulls = 54
>> Oopmap = 15872 bytes
>> 
>> calculate_oopmap: objects = 4590 (335872 bytes, 178120 bytes don't need marking), embedded oops = 46487, nulls = 29019
>> Oopmap = 10496 bytes
>> 
>> (332752 + 178120) / (507904 + 335872.0) = 0.6054592688106796 
>> More than 60% of the space used by the archived heap doesn't need to be marked by the oopmap. 
>> 
>> $ java -Xshare:dump -Xlog:cds+map | grep lead
>> [3.777s][info][cds,map] - heap_oopmap_leading_zeros: 143286
>> [3.777s][info][cds,map] - heap_ptrmap_leading_zeros: 50713
>> 
>> So we can reduce the "bm" region by (143286 + 50713) / 8 = 24249 bytes.
>> 
>> Current output:
>> $ java -XX:+UseNewCode -Xshare:dump -Xlog:cds+map | grep lead 
>> [5.339s][info][cds,map] - heap_oopmap_leading_zeros:      26
>> [5.339s][info][cds,map] - heap_ptrmap_leading_zeros:      8
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Renamed variables and moved pointer patching shift

Looks good. I have some minor remarks for refactoring.

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

> 165:   // Optimization: if dumptime shift is the same as runtime shift, we can perform a
> 166:   // quick conversion from "dumptime narrowOop" -> "runtime narrowOop".
> 167:   narrowOop* patching_start = (narrowOop*)region.start() + MetaspaceShared::oopmap_start_pos();

I think there is no need to introduce a new `MetaspaceShared::oopmap_start_pos()` API. You can get this info from


FileMapInfo::current_info()->header()->heap_oopmap_start_pos()


The same thing can be done for `MetaspaceShared::ptrmap_start_pos()`

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

> 636:   BitMap::idx_t idx = requested_field_addr - (Metadata**) _requested_bottom;
> 637:   // Leading zeros have been removed so some addresses may not be in the ptrmap
> 638:   if (idx < MetaspaceShared::ptrmap_start_pos()) {

This is now the only place you need to know `ptrmap_start_pos` during dump time (at runtime you can get the info from `FileMapInfo::current_info()`). I think it's better to store `ptrmap_start_pos` as a static field in the `ArchiveHeapWriter` class instead.

src/hotspot/share/cds/filemap.cpp line 293:

> 291:   st->print_cr("- heap_roots_offset:              " SIZE_FORMAT, _heap_roots_offset);
> 292:   st->print_cr("- _heap_oopmap_start_pos:      " SIZE_FORMAT, _heap_oopmap_start_pos);
> 293:   st->print_cr("- _heap_ptrmap_start_pos:      " SIZE_FORMAT, _heap_ptrmap_start_pos);

Need two more spaces before SIZE_FORMAT for alignment?

src/hotspot/share/cds/filemap.cpp line 1582:

> 1580:   size_t removed_zeros = old_zeros - new_zeros;
> 1581: 
> 1582:   assert(new_zeros <= old_zeros, "Should have removed leading zeros");

Maybe add this comment above:

// The start of the archived heap has many primitive arrays (String
// bodies) that are not marked by the oop/ptr maps. So we must have
// lots of leading zeros.

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17350#pullrequestreview-1904653435
PR Review Comment: https://git.openjdk.org/jdk/pull/17350#discussion_r1504859078
PR Review Comment: https://git.openjdk.org/jdk/pull/17350#discussion_r1504992213
PR Review Comment: https://git.openjdk.org/jdk/pull/17350#discussion_r1504862102
PR Review Comment: https://git.openjdk.org/jdk/pull/17350#discussion_r1504979171