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

[Coleen Phillimore]

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.

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