RFR: 8329417: Remove objects with no pointers from relocation bitmap [v3]

[Ioi Lam]

On Fri, 19 Apr 2024 17:24:08 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Both the read-write and read-only regions of the relocation bitmap contain metaspace objects with no pointers which do not need to be relocated. By sorting the maps, objects with no pointers can bubble to the top and the bitmap can trim the "leading zeros" thus reducing the archive size and reducing the number of pages. Verified with tier 1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Calvin comment and added logging

Looks good. Some suggestions to make the code easier to understand.

src/hotspot/share/cds/archiveBuilder.cpp line 81:

> 79:   // Save this source object for copying
> 80:   _objs->append(src_info);
> 81:   src_info->set_index(_objs->length()-1);

It's better to put this line before appending, so you can do


src_info->set_index(_objs->length());


That way you don't need to worry about ever getting a negative ID.

src/hotspot/share/cds/archiveBuilder.cpp line 594:

> 592: }
> 593: 
> 594: int ArchiveBuilder::compare_src_objs(SourceObjInfo** a, SourceObjInfo** b) {

I think some explanation would be useful:


  // The objects that have embedded pointers will be bubbled up
  // towards the top of the list. This ensures we have a maximum
  // number of leading zero bits in the relocation bitmap.
  if ((*a)->has_embedded_pointer() && !(*b)->has_embedded_pointer()) {
    return 1;
  } else if (!(*a)->has_embedded_pointer() && (*b)->has_embedded_pointer()) {
    return -1;
  } else {
    // This is necessary to keep the sorting order stable. Otherwise the
    // archive's contents may not be deterministic.
    return (*a)->index() - (*b)->index();
  }

src/hotspot/share/cds/archiveBuilder.hpp line 132:

> 130:     FollowMode _follow_mode;
> 131:     int _size_in_bytes;
> 132:     int _index;              // The location of this object in _source_objs

After sorting, this is no longer the location of this object in _source_objs. How about:


int _id; // Each object has a unique serial ID, starting from zero. The ID is assigned
         // when the object is added into _source_objs.

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

> 1592:     assert(new_zeros == 0, "Should have removed leading zeros");
> 1593:   )
> 1594:   assert(map->size() <= old_size, "Map size should have decreased");

The comment is not correct, as the old size could be the same as the new size. Maybe just "sanity"?

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18803#pullrequestreview-2013725901
PR Review Comment: https://git.openjdk.org/jdk/pull/18803#discussion_r1574107111
PR Review Comment: https://git.openjdk.org/jdk/pull/18803#discussion_r1574106470
PR Review Comment: https://git.openjdk.org/jdk/pull/18803#discussion_r1574100083
PR Review Comment: https://git.openjdk.org/jdk/pull/18803#discussion_r1574102431