RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v2]

[Ioi Lam]

On Tue, 7 May 2024 16:38:23 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> The beginning of the RW region contains pointers to c++ vtables which are always located at a fixed offset from the shared base address at runtime. This offset can be calculated at dumptime and stored with the read-only tables at the top of the RO region. As a further improvement, all the pointers to RO tables are replaced with offsets as well.
>> 
>> These changes will reduce the number of pointers in the RW and RO regions and will allow for the relocation bitmap size optimizations to be more effective. Verified with tier 1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Chris comments and cleanup

Looks good. I will suggest making some clean up to improve the readability of the existing code.

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

> 323:   assert((intptr_t)obj >= 0 || (intptr_t)obj < -100,
> 324:          "hit tag while initializing ptrs.");
> 325:   *p = obj != 0 ? (void*)(SharedBaseAddress + obj) : (void*)obj;

`nullptr` should be used instead of `0`. E.g., `obj != (void*)nullptr`

src/hotspot/share/cds/cppVtables.cpp line 236:

> 234:   if (!soc->reading()) {
> 235:     _vtables_serialized_base = soc->region_top();
> 236:   }

The new `region_top()` API may be confusing with the existing `do_region()` API, which has a completely different meaning for `region`.

I think it's better to rename `do_region()` to


// iterate on the pointers from p[0] through p[num_pointers-1]
SerializeClosure do_ptrs(void** p, int num_pointers);


Also, there's no need to add a new `region_top()` API -- there are already too many functions that deal with a "region" of different types, and you need to wonder what this particular "region" is.

`soc->region_top()` can be replaced with `ArchiveBuilder::current()->current_dump_space()->top()`

Also, in archiveBuilder.hpp, `dump_space` means the same thing as `dump_region`. All of the former should be changed to the latter for uniformity.

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19107#pullrequestreview-2046836828
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1594750309
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1594766914