RFR 8251557 Avoid dumping unused symbols/strings into the CDS archive
Ioi Lam
ioi.lam at oracle.com
Wed Aug 26 17:08:05 UTC 2020
Hi Calvin,
Thanks for the review.
On 8/26/20 9:40 AM, calvin.cheung at oracle.com wrote:
> Hi Ioi,
>
> The proposed change does make the code easier to read than before.
>
> I've comments on 2 files:
>
> heapShared.hpp:
>
> 42 class DumpedInternedStrings;
>
> Instead of forward declaration, is it possible to move the class
> definition there since it is defined at the end of the file?
>
I moved it as you suggested.
> archiveBuilder.hpp:
>
> 37 class DumpRegion;
>
> The above forward declaration was removed but I think it's required
> for the following:
>
I removed the forward declaration because DumpRegion is declared in
archiveUtils.hpp. Since JDK-8252056, archiveUtils.hpp is already
included by archiveBuilder.hpp:
http://hg.openjdk.java.net/jdk/jdk/rev/ba51bc0772ac
> 132 DumpRegion* _rw_region;
> 133 DumpRegion* _ro_region;
>
> Also, the following constructor needs to be moved to
> archiveBuilder.cpp or I got compile error about invalid use of
> incomplete type 'class DumpRegion'.
>
> 158 OtherROAllocMark() {
> 159 _oldtop = _singleton->_ro_region->top();
> 160 }
>
Maybe you applied the patch on a repo without the JDK-8252056 fix?
I did find a problem with archiveUtils.hpp. It was using the Klass type
without including klass.hpp, so I added
diff -r 3920d1a29976 src/hotspot/share/memory/archiveBuilder.hpp
--- a/src/hotspot/share/memory/archiveBuilder.hpp Wed Aug 19 14:14:20
2020 -0700
+++ b/src/hotspot/share/memory/archiveBuilder.hpp Wed Aug 26 10:06:59
2020 -0700
@@ -27,6 +27,7 @@
#include "memory/archiveUtils.hpp"
#include "memory/metaspaceClosure.hpp"
+#include "oops/klass.hpp"
#include "utilities/bitMap.hpp"
#include "utilities/growableArray.hpp"
#include "utilities/hashtable.hpp"
Thanks
- Ioi
> thanks,
> Calvin
>
>
> On 8/19/20 2:39 PM, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8251557
>> http://cr.openjdk.java.net/~iklam/jdk16/8251557-cds-dont-dump-unused-symbols.v01/
>>
>>
>> Part of CDS code clean up:
>>
>> Currently all entries in the SymbolTable/StringTable are dumped into
>> the CDS archive. The problems are
>>
>> (1) We end up writing many unused items, such as the mangled names
>> of hidden classes.
>> (2) We have to scan the symbol table inside a safepoint, which had
>> caused bugs before (JDK-8245264).
>>
>> Since JDK-8250990, we already maintain all used Symbols in a growable
>> array. We can dump the shared symbol table using this array, and
>> avoid walking the SymbolTable.
>>
>> For StringTable, we scan all archived classes, and dump only the
>> interned strings used by those classes (plus extra strings
>> specified by SharedArchiveConfigFile) into the CDS interned string
>> table.
>>
>> This reduces complexity in the CDS code, and reduce the size of
>> the CDS archives.
>>
>> Testing - tiers 1-4
>>
>> Archive file size for "java -Xshare:dump"
>>
>> old: 39,368 symbols 7,028 strings 11,784,200 bytes
>> new: 37,387 symbols 7,020 strings 11,683,320 bytes (~0.9% reduction)
>>
>> Thanks
>> - Ioi
>>
>>
More information about the hotspot-runtime-dev
mailing list