RFR(M) 8250990 Consolidate object copying code for CDS static/dynamic archive dumping
Ioi Lam
ioi.lam at oracle.com
Thu Aug 13 20:49:54 UTC 2020
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
>>>
>>
More information about the hotspot-runtime-dev
mailing list