RFR: 8306582: Remove MetaspaceShared::exit_after_static_dump() [v2]

[Ioi Lam]

On Tue, 25 Jul 2023 18:48:58 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.
>> 
>> Verified with tier 1-9 tests.
>
> Matias Saavedra Silva has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge fix
>  - Restores java loaders
>  - Ioi and David comments
>  - Windows fix
>  - 8306582: Remove MetaspaceShared::exit_after_static_dump()

Looks good overall. Some suggestions for code refactoring.

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

> 375:   return *(_scratch_references_table->get(src));
> 376: }
> 377: 

The logic of these two new functions are covered by the KlassToOopHandleTable class, so I would suggest refactoring   KlassToOopHandleTable to


class MetaspaceObjToOopHandleTable: public ResourceHashtable<MetaspaceObj*, OopHandle, ...


So your new "scratch_resolved_references" functions can be similar to `HeapShared::scratch_java_mirror/set_scratch_java_mirror`.

Also, this function needs to be updated


void HeapShared::remove_scratch_objects(Klass* k) {
  _scratch_java_mirror_table->remove_oop(k);
  if (k->is_instance_klass()) {
    _scratch_resolved_references_table->remove(InstanceKlass::cast(k)->constants());
  }
}

src/hotspot/share/oops/constantPool.cpp line 303:

> 301:     for (int i = 0; i < rr_len; i++) {
> 302:       oop obj = rr->obj_at(i);
> 303:       HeapShared::add_scratch_resolved_reference(orig_pool, i, nullptr);

I think the logic can be more obvious of the the management of the contents of the `scratch_rr` array is moved here. Something like:


if (rr != nullptr) {
  scratch_rr = HeapShared::scratch_resolved_references(orig_pool);
  
  .... scratch_rr->obj_at_put(i, nullptr);
  ....
  return scratch_rr;
 } 


That way, you won't have two functions with similar sounding names `add_scratch_resolved_reference` and `add_scratch_resolved_references`. HeapShared is now solely responsible for keeping track of the scratch arrays, but ConstantPool will be responsible for the contents of the scratch arrays.

Also, it's not necessary to put the `scratch_rr` in an `OopHandle` as we won't safepoint in this loop.

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

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14879#pullrequestreview-1548221065
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1275271843
PR Review Comment: https://git.openjdk.org/jdk/pull/14879#discussion_r1275259540