RFR: 8306582: Remove MetaspaceShared::exit_after_static_dump()

[Ioi Lam]

On Thu, 13 Jul 2023 21:25:17 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> Currently we exit the VM after static dumping with `MetaspaceShared::exit_after_static_dump()`. 
> 
> 
>  // We have finished dumping the static archive. At this point, there may be pending VM
>  // operations. We have changed some global states (such as vmClasses::_klasses) that
>  // may cause these VM operations to fail. For safety, forget these operations and
>  // exit the VM directly.
>  void MetaspaceShared::exit_after_static_dump() {
>    os::_exit(0);
>  } 
> 
> 
> As the comment suggests, the VM state is altered when preparing and performing the static dump, so this change aims to prevent these state changes so the VM can exit normally after the static dump completes. There are three major aspects to this change:
> 1. Since the resolved references array in the Constant Pool is altered when preparing for a static dump, a "scratch copy" is created and archived instead 
> 2. Symbols are sorted by address and have their hash recalculated. Similarly to point 1, the copies of the symbols that are to be archived have their hashes updated as opposed to the originals.
> 3. The handling of -Xshare:dump during argument parsing such that the VM can continue and exit normally with an exit code of 0.

Changes requested by iklam (Reviewer).

src/hotspot/share/cds/archiveBuilder.cpp line 262:

> 260:     // dynamic archive, we might need to sort the symbols alphabetically (also see
> 261:     // DynamicArchiveBuilder::sort_methods()).
> 262:     log_info(cds)("Sorting symbols and fixing identity hash ... ");

"and fixing identity hash" should be removed, as the has is no longer being fixed here.

src/hotspot/share/cds/archiveBuilder.cpp line 638:

> 636:   memcpy(dest, src, bytes);
> 637: 
> 638:   // Update the hash of buffered sorted symbols for static dump

Please append to the comments with ` so that the symbols have deterministic contents`

src/hotspot/share/cds/heapShared.cpp line 345:

> 343: void HeapShared::init_scratch_references() {
> 344:   if (_scratch_references_table == nullptr)
> 345:     _scratch_references_table = new (mtClass)ResolvedReferenceScratchTable();

These two lines are outside of a lock so you could run into a race condition. I think you can remove this function and move these two lines to just before calling `_scratch_references_table->put()` in `add_scratch_resolved_references`.

src/hotspot/share/cds/heapShared.hpp line 288:

> 286:     36137, // prime number
> 287:     AnyObj::C_HEAP,
> 288:     mtClassShared> ResolvedReferenceScratchTable;

You are using `oop->identity_hash()` as the key for this table. However, it's possible for two `resolved_references` arrays to have the exact same identity. It's better to to use `ResourceHashtable<OopHandle, OopHandle, ...` for this table. Then, you need to define two custom functions for these two parameters for `ResourceHashtable`


    unsigned (*HASH)  (K const&),
    bool     (*EQUALS)(K const&, K const&)

where the `K` type is `OopHandle`.

The `HASH` function can return `OopHandle::resolve()->identity_hash()` and the `EQUALS` function can compare the values of `OopHandle::resolve()`.

For the coding style, you can search for tables that use `HeapShared::oop_hash` for examples.

src/hotspot/share/classfile/classLoaderData.cpp line 1085:

> 1083:     guarantee(this == class_loader_data(cl) || has_class_mirror_holder(), "Must be the same");
> 1084:     guarantee(cl != nullptr || this == ClassLoaderData::the_null_class_loader_data() || has_class_mirror_holder(), "must be");
> 1085:   }

Why is this necessary?

src/java.base/share/native/libjli/java.c line 1447:

> 1445: /*
> 1446:  * Check for CDS option
> 1447:  */

Comments need to be indented.

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

PR Review: https://git.openjdk.org/jdk/pull/14879#pullrequestreview-1533239235
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1265645038
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1265669208
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1265653371
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1265667818
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1265648116
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1265671279