RFR 8251557 Avoid dumping unused symbols/strings into the CDS archive

[Ioi Lam]

On 8/26/20 11:13 AM, calvin.cheung at oracle.com wrote:
>
> On 8/26/20 10:18 AM, Ioi Lam wrote:
>>
>>
>> On 8/26/20 10:08 AM, Ioi Lam wrote:
>>> 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.
>>
>> Oops, sorry I hit "Send" too fast. DumpedInternedStrings depends on 
>> function declared by HeapShared, so it must be declared after HeapShared
> I see. So the change looks good.
>>
>> Thanks
>> - Ioi
>>
>>
>>>
>>>> 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?
> Right, I haven't updated my repo with the JDK-8252056 fix yet.
>>>
>>> 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"
>>>
> Since you're including klass.hpp, maybe the following forward 
> declaration can be removed?
>
> 36 class Klass;
>

Right. I will remove that.

Thanks
- Ioi

> thanks,
>
> Calvin
>
>>>
>>> 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
>>>>>
>>>>>
>>>
>>