RFR: 8307567: Avoid relocating global roots to metaspaceObjs in CDS dump

[Ioi Lam]

On Wed, 10 May 2023 18:11:14 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> The functions `ArchiveBuilder::relocate_roots()` and `ArchiveBuilder::relocate_vm_classes()` relocate global roots such as `Universe::_typeArrayKlassObjs[]` and `vmClasses::_klasses[]` to point to locations inside the CDS "buffer" space.
>> 
>> As a result, the VM is no longer usable after CDS dumping has finished.
>> 
>> Previously, we tried to save some of the VM states and restore them after CDS dump has finished. See [JDK-8264735](https://bugs.openjdk.org/browse/JDK-8264735), which added functions like `FileMapInfo::clone_shared_path_table()`.
>> 
>> However, this is too cumbersome and doesn't cover all cases. The intention of the global root relocation is to make it easy to write the roots using `WriteClosure::do_ptr()`, which could handle only pointers into the "buffer" space. If we call `get_buffered_addr()` inside `WriteClosure::do_ptr()`, then there's no need to modify the global roots.
>> 
>> Notes for reviewers:
>> 
>> - Start with changes in `WriteClosure::do_ptr()`
>> - Some code in SystemDictionaryShared also needed to call `get_buffered_addr()`
>> - I added new function `ArchiveBuilder::write_pointer_in_buffer()` to simplify writing pointers into the CDS buffer
>> -  `ArchiveBuilder::relocate_roots()`, `ArchiveBuilder::relocate_vm_classes()` and `FileMapInfo::clone_shared_path_table()` are removed.
>>     - I am leaving `CloneDumpTimeClassTable` and `CloneDumpTimeLambdaProxyClassTable` for now. I plan to remove them in a follow up RFE after more testing.
>
> src/hotspot/share/cds/archiveBuilder.cpp line 458:
> 
>> 456: 
>> 457: void ArchiveBuilder::remember_embedded_pointer_in_gathered_obj(address src_obj, address* field_addr) {
>> 458:   // field_addr must be a valid field inside src_obj.
> 
> Is there a way to check this with an assert?

The assert is done inside `ArchiveBuilder::SourceObjList::remember_embedded_pointer()`, which knows the size of `src_obj`:


assert(field_offset_in_bytes >= 0, "must be");
assert(field_offset_in_bytes + intx(sizeof(intptr_t)) <= intx(src_obj_size), "must be");

> src/hotspot/share/memory/iterator.hpp line 359:
> 
>> 357:   }
>> 358: 
>> 359:   // Useful aliases
> 
> This is a nice addition but it might be buried under the rest of this PR. Would it be worth making this a separate enhancement?

I filed [JDK-8307959](https://bugs.openjdk.org/browse/JDK-8307959): "Remove explicit type casts from SerializeClosure::do_xxx() calls"

 I will integrate that before integrate this PR.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13854#discussion_r1191624287
PR Review Comment: https://git.openjdk.org/jdk/pull/13854#discussion_r1191624253