RFR: 8250989: Consolidate buffer allocation code for CDS static/dynamic dumping [v2]

[Thomas Stuefe]

On Tue, 2 Feb 2021 00:02:41 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> src/hotspot/share/memory/metaspace.cpp line 681:
>> 
>>> 679: 
>>> 680:   // If UseCompressedClassPointers=1, we have two cases:
>>> 681:   // a) if CDS archive has been mapped, it will create the ccs
>> 
>> This comment is confusing. So, at this point the ccs archive has already be mapped? Where?
>
> I've changed it to:
> 
>   // a) if CDS archive(s) have been mapped, we would have already reserved the
>   //    ccs (immediately above the mapped archives) and initialized
>   //    the CompressedKlassPointers encoding.

No, the confusing part is the order. The old comment used future tense, this uses past tense. So to the reader it looks like mapping may already have happened. But at this point in the code the archive has not been mapped yet, no?
Proposal:
  // If UseCompressedClassPointers=1, we have two cases:
  // a) if CDS is active (runtime, Xshare=on), it will create the class space
  //    for us, initialize it and set up CompressedKlassPointers encoding.
  //    Class space will be reserved above the mapped archives.
  // b) if CDS either deactivated (Xshare=off) or a static dump is to be done (Xshare:dump),
  //    we will create the class space on our own. It will be placed above the java heap,
  //    since we assume it has been placed in low
  //    address regions. We may rethink this (see JDK-8244943). Failing that,
  //    it will be placed anywhere.

>> src/hotspot/share/memory/metaspaceShared.cpp line 283:
>> 
>>> 281:       _extra_symbols->append(SymbolTable::new_permanent_symbol(utf8_buffer));
>>> 282:     } else{
>>> 283:       assert(prefix_type == HashtableTextDump::StringPrefix, "Sanity");
>> 
>> So this whole comment is wrong now and obsolete?
>
> Yes, most of the work done under the old function `MetaspaceShared::initialize_dumptime_shared_and_meta_spaces()` are now unnecessary. Specifically, it doesn't initialize the "shared space" or "meta space" anymore. That's why I renamed it to `MetaspaceShared::initialize_for_static_dump()`. 
> 
> Now it only does the following:
> - reset `SharedBaseAddress` if too high
> - reserve `_symbol_rs`
> 
> ===============
> BTW, I have added comments at the top of archiveBuilder.hpp to give an overview of how the memory is reserved and relocated during the dump process.

Thanks for the clarification.

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

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