RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Coleen Phillimore]

On 11/2/15 1:57 PM, Ioi Lam wrote:
>
>
> On 10/30/15 1:31 PM, Coleen Phillimore wrote:
>>
>>
>> On 10/30/15 4:18 PM, Karen Kinnear wrote:
>>> Coleen,
>>>
>>> Question for you below please ...
>>>> On Oct 30, 2015, at 3:44 PM, Coleen Phillimore 
>>>> <coleen.phillimore at oracle.com> wrote:
>>>>
>>>>
>>>> Hi Ioi,
>>>> This is a manageable code change.
>>>>
>>>> http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html 
>>>>
>>>>
>>>> You forward declare Klass* but don't use it in this header file.
>>>> Also can you add a comment to #endif  to say what it's endifing. 
>>>> ie. // SHARE_VM_MEMORY_CLASSLISTPARSER_HPP
>>>>
>>>> http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html 
>>>>
>>>>
>>>>   33   TempNewSymbol class_name_symbol = 
>>>> SymbolTable::new_permanent_symbol(parser->current_class_name(), 
>>>> THREAD);
>>>>
>>>> This doesn't make sense.   If you want a permanent symbol, it 
>>>> doesn't need to get un-reference counted with the TempNewSymbol 
>>>> destructor.
>>> Thank you for chiming in on this one - I wanted your opinion here. 
>>> (this code used to be in MetaspaceShared::
>>> preload_and_dump and I think was wrong there as well).
>>> My understanding is that it is a TempNewSymbol that he wants, so he 
>>> should call SymbolTable::new_symbol.
>>> The code creates a (temporary) symbol for the name, and then calls 
>>> SystemDictionary::resolve_or_null, which if it
>>> succeeds will walk through the classFileParser which will create a 
>>> permanent symbol for the class name,
>>> via the symbol_alloc_batch handling. That would be consistent with 
>>> the TempNewSymbol call in
>>> SystemDictionary::resolve_or_null as well as in parse_stream - all 
>>> places dealing with the class name
>>> before doing classfile parsing.
>>>
>>> Does that make sense?
>>
>> Yes, this makes sense.  The symbol shouldn't be permanent.   Ioi can 
>> test this by putting a strange long name in the classlist file and 
>> make sure it doesn't make it out to the shared archive, since I think 
>> -Xshare:dump cleans out the SymbolTable before dumping.
>>
> After changing to use new_symbol instead of new_permanent_symbol, I 
> ran -Xshare:dump:
>
> $ java -Xshare:dump
> Preload Warning: Cannot find sun/text/normalizer/UnicodeMatcher
> ...
> total   :  13063134 [100.0% of total] out of  27385856 bytes [47.7% used]
>
> $ strings images/jdk/lib/i386/hotspot/classes.jsa | grep 
> sun/text/normalizer/UnicodeMatcher
> sun/text/normalizer/UnicodeMatcher
>
> So the unused symbols are not removed. Anyway, I have filed a separate 
> issue for this: https://bugs.openjdk.java.net/browse/JDK-8141207

I just realized that the reason that it doesn't clean out unused symbols 
is that it would leave gaps in the Metaspace memory where they're 
stored, so there wouldn't be much point.

Coleen

>
> Thanks
> - Ioi
>
>
>> Coleen
>>>
>>> thanks,
>>> Karen
>>>
>>>> http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html 
>>>>
>>>>
>>>> +    // Make sure we have an entry in the SystemDictionary on success
>>>>
>>>>
>>>> This assert code is a copy of some code elsewhere.  Can you make it 
>>>> a function that they boh can call?
>>>> Can you also comment the raw #endif's to what they're endifing?
>>>>
>>>> Otherwise, this looks okay.
>>>>
>>>> Coleen
>>>>
>>>>
>>>> On 10/30/15 1:00 PM, Ioi Lam wrote:
>>>>> Please review the following fix:
>>>>>
>>>>> http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/
>>>>>
>>>>> Bug: Clean up and refactor of class loading code for CDS
>>>>>
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8140802
>>>>>
>>>>> Summary of fix:
>>>>>
>>>>>     We need to clean up and refactor the class loading code in order
>>>>>     to support CDS in JDK9
>>>>>
>>>>>     [1] Remove code that has been made obsolete by the module changes
>>>>>         (such as supporting code used for meta-index file)
>>>>>     [2] Add new whitebox API to be used by CDS-related tests.
>>>>>     [3] Refactor the parsing of classlist files for future 
>>>>> enhancements.
>>>>>     [4] Add new APIs in the class loading code to support Oracle 
>>>>> CDS enhancements.
>>>>>
>>>>> Tests:
>>>>>
>>>>>     JPRT
>>>>>     RBT - with same set of tests as hs-rt nightly
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>
>