RFR: 8329416: Split relocation pointer map into read-write and read-only maps [v2]

[Ioi Lam]

On Mon, 8 Apr 2024 21:39:32 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> The pointer relocation bitmap is already subdivided into read-write and read-only regions and can be split into two distinct bitmaps. By separating the maps, each sub-map can be further optimized. Verified with tier 1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Ioi comments and cleanup

I have more suggestions for cleaning up the code.

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

> 296:   st->print_cr("- has_full_module_graph           %d", _has_full_module_graph);
> 297:   st->print_cr("- rw_ptrmap_size_in_bits:         " SIZE_FORMAT, region_at(MetaspaceShared::rw)->ptrmap_size_in_bits());
> 298:   st->print_cr("- ro_ptrmap_size_in_bits:         " SIZE_FORMAT, region_at(MetaspaceShared::ro)->ptrmap_size_in_bits());

Since this information is inside `FileMapRegion`, I think it's better to modify `FileMapRegion::print()` to print out the `_ptrmap_offset` and `_ptrmap_size_in_bits`. We already print the information for the `oopmap` there.

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

> 1918:                           p2i(bitmap_base), rw_ptrmap.size());
> 1919:     log_debug(cds, reloc)("mapped relocation ro bitmap @ " INTPTR_FORMAT " (" SIZE_FORMAT " bits)",
> 1920:                           p2i(ro_bitmap_base), ro_ptrmap.size());

As we are using `BitMapView` in various cases (ptrmaps for RO and RW regions, and ptrmap/oopmap for HP region), I think we should simplify and unify the code.

The address of `ro_bitmap_base` and `bitmap_base` are internal to `FileMapRegion::bitmap_view()`, so we should move the logging inside that function.

I would suggest doing this:


BitMapView FileMapRegion::bitmap_view(bool is_oopmap, const char* name) {
  char* bitmap_base = FileMapInfo::current_info()->map_bitmap_region();
  bitmap_base += is_oopmap ? _oopmap_offset : _ptrmap_offset;
  size_t size_in_bits = is_oopmap ? _oopmap_size_in_bits : _ptrmap_size_in_bits;
  log_debug(cds, reloc)("mapped %s relocation %smap @ " INTPTR_FORMAT " (" SIZE_FORMAT " bits)",
                        name, is_oopmap ? "oop" : "ptr",
                        p2i(bitmap_base), size_in_bits);
  return BitMapView((BitMap::bm_word_t*)(bitmap_base), size_in_bits);
}


It's a shame that we have to pass in the name of the region, as a `FileMapRegion` doesn't know what region it is. I think we can remove these two functions:


BitMapView FileMapRegion::oopmap_view() {
  return bitmap_view(true);
}

BitMapView FileMapRegion::ptrmap_view() {
  assert(has_ptrmap(), "must be");
  return bitmap_view(false);
}
``` 

and add two corresponding functions in `FileMapInfo` (which knows the names of its regions):


BitMapView FileMapInfo::oopmap_view(int region_index) {
  return region_at(region_index)->bitmap_view(/*is_oopmap*/true, region_name(region_index));
}

BitMapView FileMapInfo::ptrmap_view(int region_index) {
  return region_at(region_index)->bitmap_view(/*is_oopmap*/false, region_name(region_index));
}


The assert can be moved into FileMapRegion::bitmap_view:


  if (is_oopmap) {
    assert(has_oopmap(), "sanity");
  } else {
    assert(has_ptrmap(), "sanity");
   }


There are a few places that we construct the BitMapView by hand. E.g.,


void ArchiveHeapLoader::patch_embedded_pointers(FileMapInfo* info,
                                                MemRegion region, address oopmap,
                                                size_t oopmap_size_in_bits) {
  BitMapView bm((BitMap::bm_word_t*)oopmap, oopmap_size_in_bits);


The `oopmap` and `oopmap_size_in_bits` parameters should be removed and this function should call


BitMapView vm = info->oopmap(MetaspaceShared::hp);


There's a similar issue with `ArchiveHeapLoader::load_heap_region_impl()`.

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18608#pullrequestreview-1987760747
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1556595616
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1556650939