RFR(M) 8250990 Consolidate object copying code for CDS static/dynamic archive dumping

[Coleen Phillimore]

Ioi, This version looks better.  I still think there's too many internal details that are exposed in archiveBuilder.hpp, but this can be cleaned up in another pass.

Can you fix the typo: _estimated_metsapceobj_bytes

Otherwise, this is reviewed.
Coleen


----- Original Message -----
From: ioi.lam at oracle.com
To: hotspot-runtime-dev at openjdk.java.net
Sent: Tuesday, August 4, 2020 1:59:27 AM GMT -05:00 US/Canada Eastern
Subject: Re: RFR(M) 8250990 Consolidate object copying code for CDS static/dynamic archive dumping

Here's an updated version after offline discussion with Coleen:

http://cr.openjdk.java.net/~iklam/jdk16/8250990-consolidate-static-dynamic-cds-objcopy.v02/
http://cr.openjdk.java.net/~iklam/jdk16/8250990-consolidate-static-dynamic-cds-objcopy.v02-delta/

- Moved DumpAllocStats into its own cpp/hpp files
- Removed the use of inline.hpp files from archiveBuilder.hpp
- Changed comments about the old ArchiveCompactor class to
   the new AcrchiveBuilder class.
- Removed CDSAddSymbol_lock which is no longer used.

Thanks
- Ioi

On 8/3/20 3:11 PM, Ioi Lam wrote:
> There turns out to be a lot of duplication in the static/dynamic 
> dumping code. To keep the changes manageable, I've reorganized the REF 
> into a few sub-tasks. This particular review is for:
>
> https://bugs.openjdk.java.net/browse/JDK-8250990
> http://cr.openjdk.java.net/~iklam/jdk16/8250990-consolidate-static-dynamic-cds-objcopy.v01/ 
>
> https://wiki.openjdk.java.net/display/HotSpot/How+CDS+Copies+Class+Metadata+into+the+Archive 
>
>
> I've also updated the e-mail subject to use the new JBS ID (JDK-8250990).
>
> This patch is an intermediate step. Some code might look like they are 
> in the wrong place -- for example: _estimated_metsapceobj_bytes has 
> been moved to archiveBuilder.hpp, but _estimated_hashtable_bytes is 
> still in dynamicArchive.cpp. The latter will eventually be moved as 
> part of the second step (JDK-8250989
> Consolidate buffer allocation code for CDS static/dynamic archive 
> dumping).
>
> (Coleen has given me feedback off-line. I'll post a new version soon 
> to limit the API in archiveBuilder.hpp)
>
> Thanks
> - Ioi
>
> On 7/31/20 11:36 PM, Ioi Lam wrote:
>>
>> Currently there are two very similar implementations for static CDS 
>> archive
>> dumping (metaspaceShared.cpp) and dynamic CDS archive dumping 
>> (dynamicArchive.cpp).
>>
>> I've merged the two versions and moved to a new file, 
>> archiveBuilder.cpp.
>>
>> I also improved the copying algorithm to speed up dumping. Times for 
>> dumping
>> 20000+ classes:
>>
>>            static        dynamic
>>     Old    42.655 sec    67.014 sec
>>     New    37.027 sec    34.974 sec
>>
>> Normally I would not try to optimize when refactoring code. I made an
>> earlier, naive attempt to use the old dynamic dumper for static dumping
>> (because dynamic dumping is essentially a superset of static dumping).
>> However, that caused significant regression in static dumping speed.
>>
>> Hopefully the new algorithm is both faster and easier to understand.
>> Please see the wiki document for more information.
>>
>> Tested with mach5 tiers 1-4.
>>
>> Thanks
>> - Ioi
>