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