On 12/08/2014 4:29 PM, Ioi Lam wrote:
On 8/10/14, 7:16 PM, David Holmes wrote:
Hi Ioi,
We seem to have lost core-libs-dev so I added them back.
A couple of minor follow ups.
On 9/08/2014 6:02 PM, Ioi Lam wrote:
Hi,
Thanks a lot to everyone for the very useful comments. I have updated the webrev
Just the delta from the previous round of review:
http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3_delta_from_v2/
JDK changes:
URLClassLoader.java:
Doesn't this Note
+ * + * NOTE: the logic used here has been duplicated in the VM native code + * (search for invocation of definePackageInternal in the HotSpot sources). + * If this is changed, the VM code also need to be modified.
belong on definePackageInternal, not defineClass ?? Yes, this is a little confusing since the comment is lacking context ... I will try to reword it. ---
hotspot changes:
hotspot/src/share/vm/classfile/classLoader.cpp
The scoping of the ResourceMark doesn't look right:
592 // Iterate over class path entries 593 for (int start = 0; start < len; start = end) { 594 while (class_path[end] && class_path[end] != os::path_separator()[0]) { 595 end++; 596 } 597 EXCEPTION_MARK; 598 ResourceMark rm(THREAD); 599 char* path = NEW_RESOURCE_ARRAY(char, end-start+1); 600 strncpy(path, &class_path[start], end-start); 601 path[end-start] = '\0'; 602 update_class_path_entry_list(path, false); 603 #if INCLUDE_CDS 604 if (DumpSharedSpaces) { 605 check_shared_classpath(path); 606 } 607 #endif 608 while (class_path[end] == os::path_separator()[0]) { 609 end++; 610 } 611 }
Doesn't the RESOURCE_ARRAY need to be freed within the ResourceMark block?
I want the path to be freed after each iteration of the "for" loop. Is this the right way to do it? I though the path will be freed once "rm" falls out of scope, which happens after each iteration of the "for" loop is finished.
You're right - I'm getting myself confused. (Some of our "marks" do things, while others check things - I thought this was a checker.)
---
src/share/vm/runtime/arguments.cpp
3340 // This causes problems with CDS, which requires that all directories specified in the classpath 3341 // must be empty.
Should that be "must not be empty"? Or did you mean directory names?
As mentioned by Jiangli, we want the directory to be empty, so no classes will be loaded from them at run-time.
Got it. I got confused with the path entry and the directory referred to by a path entry. Thanks, David
A little history:
CDS supports only loading classes from JAR files. This way, we can check if a JAR file has been modified using its timestamp and size. If the JAR file has been modified, the JVM will declare the CDS archive out of date and refuse to use it. This way, we can ensure that the set of loadable classes for our ClassLoader will not change between dump-time and run-time.
Directories are more problematic than JAR files -- there's no easy way to check if a class file has been added or deleted (unless you do an expensive directory scan). So since JDK 1.5 we have forbidden the use of non-empty directories for CDS.
Empty directories have been allowed since JDK 1.5, probably because the development JDK builds includes $JAVA_HOME/somewhere/classes in its bootclasspath. We are just carrying this vestige forward, although I am thinking of filing a bug to get rid of it.
---
src/share/vm/runtime/javaCalls.cpp
+ // may cause undesriable side-effect in the class metadata.
Typo: undesriable; also side-effects
Fixed.
Thanks - Ioi
---
David -----
All the changes:
http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/
Thanks - Ioi
On 7/28/14, 4:09 PM, Ioi Lam wrote:
Hi Folks,
Please review the following clean up and refactoring of the CDS code, for JDK9
http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v2/ https://bugs.openjdk.java.net/browse/JDK-8046070
Summary of fix:
Clean up and refactor the Class Data Sharing (CDS) code, including:
+ Improve archive integrity checking + Support bytecode verification during archive dumping time + Allow the user to configure the CDS class list and archive file. + Allow future extension of the CDS class loading mechanism.
Tests:
JPRT UTE (vm.runtime.testlist, vm.quick.testlist, vm.parallel_class_loading.testlist) Various adhoc SQE tests on Aurora JCK
Thanks - Ioi