On Thu, 13 Jul 2023 21:25:17 GMT, Matias Saavedra Silva <[email protected]>
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
