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

[calvin.cheung at oracle.com]

Hi Ioi,

This looks pretty good.

One question on archiveBuilder.cpp:

  246 int ArchiveBuilder::compare_symbols_by_address(Symbol** a, 
Symbol** b) {
  247   if (a[0] < b[0]) {
  248     return -1;
  249   } else {
  250     assert(a[0] > b[0], "Duplicated symbol %s unexpected", 
(*a)->as_C_string());
  251     return 1;
  252   }
  253 }

It seems the (a[0] == b[0]) case and a ResourceMark are missing. The old 
code in metaSpaceShared.cpp has the following:

  669 static int compare_symbols_by_address(Symbol** a, Symbol** b) {
  670   if (a[0] < b[0]) {
  671     return -1;
  672   } else if (a[0] == b[0]) {
  673     ResourceMark rm;
  674     log_warning(cds)("Duplicated symbol %s unexpected", 
(*a)->as_C_string());
  675     return 0;
  676   } else {
  677     return 1;
  678   }
  679 }

thanks,

Calvin

On 8/3/20 10:59 PM, Ioi Lam wrote:
> 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
>>
>