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

[Ioi Lam]

Hi Calvin,

Thanks for the review. It's not possible for a[0] == b[0] because the 
array is built using UniqueMetaspaceClosure, which ensures that we will 
not visit see two Symbols with the same address. This condition is 
caught by the assert(a[0] > b[0]).

When this function is called, we are already inside a ResourceMark, so 
(*a)->as_C_string() will be able to allocate from the resource area. 
Such an allocation will happen only if a[0] == b[0], and afterwards the 
VM will abort, so there's no need (and it's cleaner) to not use a 
ResourceMark here.

Thanks
- Ioi

On 8/13/20 12:12 PM, calvin.cheung at oracle.com wrote:
> 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
>>>
>>