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