RFR: 8255495: Support CDS Archived Heap for uncompressed oops [v2]

[Calvin Cheung]

On Wed, 26 Jan 2022 22:14:20 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @iklam comments
>
> src/hotspot/share/cds/filemap.cpp line 2151:
> 
>> 2149:     HeapShared::set_runtime_delta(delta);
>> 2150:   }
>> 2151: 
> 
> I think the code in the previous few blocks can be simplified by adding two utility functions:
> 
> 
> // The address where the bottom of this shared heap region should be mapped
> // at runtime
> address FileMapInfo::heap_region_runtime_start_address(FileMapRegion* spc) {
>   assert(UseSharedSpaces, "runtime only");
>   spc->assert_is_heap_region();
>   if (UseCompressedOops) {
>     return start_address_as_decoded_from_archive(si);
>   } else {
>      header()->heap_begin() + si->mapping_offset() + HeapShared::set_runtime_delta();
>   }
> }
> 
> void FileMapInfo::set_shared_heap_runtime_delta(ptrdiff_t delta) {
>   if (UseCompressedOops) {
>     HeapShared::init_narrow_oop_decoding(narrow_oop_base() + delta, narrow_oop_shift());
>   } else {
>     HeapShared::set_runtime_delta(delta);
>   }
> }
> 
> 
> Then, many of the `if (UseCompressedOops)` checks above can be eliminated.

For `FileMapInfo::heap_region_runtime_start_address`, I have modified it slightly to the following because
the first time it is called `_runtime_delta` hasn't been set yet.


address FileMapInfo::heap_region_runtime_start_address(FileMapRegion* spc, ptrdiff_t delta) {
  assert(UseSharedSpaces, "runtime only");
  spc->assert_is_heap_region();
  if (UseCompressedOops) {
    return start_address_as_decoded_from_archive(spc);
  } else {
    assert(is_aligned(spc->mapping_offset(), sizeof(HeapWord)), "must be");
    return header()->heap_begin() + spc->mapping_offset() + delta;
  }
}

> src/hotspot/share/cds/filemap.cpp line 2211:
> 
>> 2209:         assert(is_aligned(si->mapping_offset(), sizeof(HeapWord)), "must be");
>> 2210:         start = (HeapWord*)(header()->heap_begin() + si->mapping_offset() + HeapShared::runtime_delta());
>> 2211:       }
> 
> This block can also be replaced with
> 
> 
> HeapWord* start = (HeapWord*)heap_region_runtime_start_address(si);
> 
> The `is_aligned(si->mapping_offset()` check can be moved into `heap_region_runtime_start_address()`

Done.

> src/hotspot/share/cds/heapShared.hpp line 425:
> 
>> 423:   static void clear_root(int index);
>> 424: 
>> 425:   static void set_runtime_delta(ptrdiff_t offset) {
> 
> For consistency, `offset` should be renamed to `delta`.

Done.

> src/hotspot/share/runtime/arguments.cpp line 3454:
> 
>> 3452: 
>> 3453:   if (DumpSharedSpaces && !UseCompressedOops) {
>> 3454:     FLAG_SET_CMDLINE(MaxHeapSize, (size_t)4 * G);
> 
> I think we should override MaxHeapSize only if it's greater than 4G. The user should be able to choose a smaller value.

Per our off list discussions, the InitialHeapSize, MinHeapSize, and MaxHeapSize are being set in
`universe_init()` via `MetaspaceShared::adjust_heap_sizes_for_dumping()`.

> test/hotspot/jtreg/runtime/cds/appcds/javaldr/HumongousDuringDump.java line 31:
> 
>> 29:  * @requires vm.cds.write.archived.java.heap
>> 30:  * @requires vm.jvmti
>> 31:  * @requires vm.opt.final.UseCompressedOops == true
> 
> Why is this change needed?

I ran the test with the -XX:-UseCompressedOops and without the change and it passed. I've reverted the change.

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

PR: https://git.openjdk.java.net/jdk/pull/7223