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

[Ioi Lam]

Hi Yumin,

Thanks for the review.

On 8/26/20 1:44 PM, Yumin Qi wrote:
>
> 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.

Actually hash is used a few lines down, so it can't be removed:

753     _writer->add(hash, CompressedOops::encode(new_s));

Thanks
- Ioi

> 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
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>