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

[Yumin Qi]

Hi, Ioi

    Looks good to me. One minor comment:

    stringTable.cpp:

746     unsigned int hash = java_lang_String::hash_code(s);

   hash is not used so this can be removed.
   No new webrev needed.

Thanks
Yumin

On 8/26/20 11:30 AM, Ioi Lam wrote:
>
>
> 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
>>>>>>
>>>>>>
>>>>
>>>
>