RFR(M) 8250990 Consolidate object copying code for CDS static/dynamic archive dumping
calvin.cheung at oracle.com
calvin.cheung at oracle.com
Thu Aug 13 19:12:33 UTC 2020
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