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

[Ioi Lam]

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

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
>