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

[Thomas Stuefe]

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

>> Before this PR, when a static CDS archive is dumped, we would unconditionally allocate a 4GB ReservedSpace and use that as the buffer for writing the archive content. We usually don't need such a big ReservedSpace. this also unnecessarily complicates Metaspace::global_initialize().
>> 
>> After this PR, both we first load all the classes and then allocate a buffer with an appropriate size.
>> 
>> I also simplified the pointer relocation code that prepares the archive to be mapped at the "requested location" (usually 0x800000000), and improved the comments.
>> 
>> We used to have lots of special cases for adjusting pointers during dynamic dump. All of those are removed.
>> 
>> Reviewers, please start with
>> - Header comments in archiveBuilder.hpp that describe the overall process of archive dumping
>> - `MetaspaceShared::initialize_for_static_dump()`
>> - `ArchiveBuilder::reserve_buffer()`
>> - Comments around `RelocateBufferToRequested` in archiveBuilder.cpp
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   @calvinccheung and @tstuefe comments

Hi Ioi,

not a full review, but the metaspace related changes to reservations look fine. See minor nits below.

This is unrelated to your patch: in compute_shared_base, you set SharedBaseAddress in case of an error. I think it should either be just a calculator function with no side effects - just returning the corrected value - or always set SharedBaseAddress and _requested_base_address always, returning void. That would be clearer.

One question, SharedBaseAddress vs MetaspaceShared::_requested_base_address : what is the difference? When do you use one, when the other?

Thanks, Thomas

src/hotspot/share/memory/metaspace.cpp line 690:

> 688: 
> 689: #if INCLUDE_CDS
> 690:   // case (a)

I'd like to preserve this comment. It makes it easier to understand why the followup check needs to query UseSharedSpaces again instead of being placed in the else path.

src/hotspot/share/oops/oop.cpp line 147:

> 145: 
> 146: #if INCLUDE_CDS_JAVA_HEAP
> 147: void oopDesc::set_narrow_klass(narrowKlass nk) {

Make private and access via a CDS-local friend class?

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

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