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

[Ioi Lam]

On Wed, 3 Apr 2024 20:01:20 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.

This looks good. I have some suggestions to simplify the code.

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

> 207:   CHeapBitMap _ptrmap;    // bitmap used by ArchivePtrMarker
> 208:   CHeapBitMap _rw_ptrmap;    // bitmap used by ArchivePtrMarker
> 209:   CHeapBitMap _ro_ptrmap;    // bitmap used by ArchivePtrMarker

I think we should add some comments:


   // Combined bitmap to track pointers in both RW and RO regions. This is updated
   // as objects are copied into RW and RO.
  CHeapBitMap _ptrmap;

  // _ptrmap is split into these two bitmaps which are written into the archive.
  CHeapBitMap _rw_ptrmap;   // marks pointers in the RW region
  CHeapBitMap _ro_ptrmap;   // marks pointers in the RO region

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

> 100:   }
> 101:   assert(_ptrmap->size() - ro_start == _ro_ptrmap->size(), "must be");
> 102:   // Free _ptrmap?

We can add a `ArchivePtrMarker::dispose()` function that frees all 3 bitmaps.

src/hotspot/share/cds/archiveUtils.inline.hpp line 37:

> 35: 
> 36:   address old_ptr = *p;
> 37: 

Unintentional change?

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

> 1613:   header()->set_rw_ptrmap_size_in_bits(rw_ptrmap->size());
> 1614:   written = write_bitmap(ro_ptrmap, buffer, written);
> 1615:   header()->set_ro_ptrmap_size_in_bits(ro_ptrmap->size());

We actually have fields in `CDSFileMapRegion` that record the size and starting position of a bitmap inside the `bm` region: (the comments for `_ptrmap_offset` needs to be updated).


typedef struct CDSFileMapRegion {
...
  size_t  _ptrmap_offset;     // Bitmap for relocating native pointer fields in archived heap objects.
                              // (The base address is the bottom of the BM region).
  size_t  _ptrmap_size_in_bits;


I think these can be reused to store the information for the ro_ptrmap and rw_ptrmap.


  region_at(MetaspaceShared::rw)->init_oopmap(0, rw_ptrmap->size());
  written = write_bitmap(rw_ptrmap, buffer, written);

  region_at(MetaspaceShared::ro)->init_oopmap(written, ro_ptrmap->size());
  written = write_bitmap(ro_ptrmap, buffer, written);

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

> 1913:     BitMapView rw_ptrmap((BitMap::bm_word_t*)bitmap_base, rw_ptrmap_size_in_bits);
> 1914:     char* ro_bitmap_base = bitmap_base + rw_ptrmap.size_in_bytes();
> 1915:     BitMapView ro_ptrmap((BitMap::bm_word_t*)ro_bitmap_base, ro_ptrmap_size_in_bits);

There's no guarantee that `FileMap::write_bitmap` will actually write only `rw_ptrmap.size_in_bytes()`, as it could do some padding as well.

If we use the `init_oopmap()` call as suggested above, then the above two lines can be replaced by a call to `FileMapRegion::bitmap_view(false)`.

We should do the same for the RW region as well. This way, we can delegate the BitMapView creation to common code.

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18608#pullrequestreview-1985374317
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1555177070
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1555177734
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1555178437
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1555182518
PR Review Comment: https://git.openjdk.org/jdk/pull/18608#discussion_r1555184977