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

[Ioi Lam]

On Wed, 10 Apr 2024 19:40:26 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - Fixed for dynamic dump
>  - Merge branch 'master' into split_ptrmap_8329416
>  - Ioi comments and cleanup
>  - Removed debugging code
>  - Merge branch 'master' into split_ptrmap_8329416
>  - Corrected ro bitmap size and start
>  - Two maps at runtime
>  - Rw and Ro bitmaps read as a single bitmap at runtime
>  - Removed debugging code
>  - Split relocation pointer map into read-write and read-only maps

I think the common bitmap_view() function can be further simplified by moving it into the FileMapInfo class, like this:


BitMapView FileMapInfo::bitmap_view(int region_index, bool is_oopmap) {
  char* bitmap_base = map_bitmap_region();
  assert(bitmap_base != nullptr, "must have been mapped already");

  FileMapRegion* r = region_at(region_index);
  const char* name = region_name(region_index);

  bitmap_base += is_oopmap ? r->oopmap_offset() : r->ptrmap_offset();
  size_t size_in_bits = is_oopmap ? r->oopmap_size_in_bits() : r->ptrmap_size_in_bits();
  log_debug(cds, reloc)("mapped %s relocation %smap @ " INTPTR_FORMAT " (" SIZE_FORMAT " bits)",
                        region_name(region_index), is_oopmap ? "oop" : "ptr",
                        p2i(bitmap_base), size_in_bits);
  return BitMapView((BitMap::bm_word_t*)(bitmap_base), size_in_bits);
}

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

src/hotspot/share/cds/archiveUtils.cpp line 86:

> 84:   // E.g., if rw_bottom = (address*)100
> 85:   //          ro_bottom = (address*)116
> 86:   //       then ro_bottom - rw_bottom = (116 - 100) / sizeof(address) = 4;

Should be

//      then for 64-bit platform:
//          ro_start = ro_bottom - rw_bottom = (116 - 100) / sizeof(address) = 2;

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

> 1608:   // The bitmap region contains up to 4 parts:
> 1609:   // rw_ptrmap:              metaspace pointers inside the read-write region
> 1610:   // ro_ptrmap:              metaspace pointers inside the read-only region

Align with the following two lines.

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

> 1923:     address rw_patch_end  = (address)rw_region->mapped_end();
> 1924:     address ro_patch_base = (address)ro_region->mapped_base();
> 1925:     address ro_patch_end  = (address)mapped_end();

I think it's better to use this -- so the code doesn't need to assume that rw is at the beginning of the mapped region, and ro is at the end. Also, the comment at line 1921 should be replaced with


    // Patch all pointers inside the RW region
    address rw_patch_base = (address)rw_region->mapped_base();
    address rw_patch_end  = (address)rw_region->mapped_end();
    
    // Patch all ponters inside the RO region
    address ro_patch_base = (address)ro_region->mapped_base();
    address ro_patch_end  = (address)ro_region->mapped_end();

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18608#pullrequestreview-1992738824
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1560105691
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1560097151
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1560097186