Coleen, Question for you below please ... > On Oct 30, 2015, at 3:44 PM, Coleen Phillimore <coleen.phillim...@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? 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 >
