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

[Ioi Lam]

On Mon, 11 Mar 2024 20:40:24 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:
> 
>   Removed metaspaceshared changes

LGTM. Just some small nits.

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

> 1582: 
> 1583:   assert(new_zeros == 0, "Should have removed leading zeros");
> 1584:   assert(map->size_in_bytes() <= old_size, "Map size should have decreased");

The comment doesn't match the code, since you are comparing with `<=`. I think it's safe to change to `<` as we should have removed more than 64 leading bits, due to the abundance of primitive arrays.

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

> 1157: WalkOopAndArchiveClosure* WalkOopAndArchiveClosure::_current = nullptr;
> 1158: 
> 1159: class PointsToOopsChecker : public BasicOopIterateClosure {

This class needs a short comment. How about:

// Checks if an oop has any non-null oop fields

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

Marked as reviewed by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17350#pullrequestreview-1930118444
PR Review Comment: https://git.openjdk.org/jdk/pull/17350#discussion_r1520863009
PR Review Comment: https://git.openjdk.org/jdk/pull/17350#discussion_r1520866317