On Tue, 7 May 2024 16:38:23 GMT, Matias Saavedra Silva <[email protected]>
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
