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