On 8/11/14 2:15 PM, Ioi Lam wrote:

...

http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/

I would like to avoid adding private methods for VM to call as fewer as possible. SecureClassLoader.getProtectionDomain(URL) Can you use the existing private getProtectionDomain(CodeSource) method instead? The VM can call the public CodeSource(URL, CodeSigner[]) constructor. I doubt an additional up call from the VM to Java may cause performance degradation as much while I think it's a good tradeoff. It also allows this be extended to signed JARs if it's considered. Manifest.getManifest(byte[] buf) Is saving one additional upcall from VM to Java very critical? I prefer the VM to use public APIs to minimize such dependency in the library. Launcher.getFileURL(String) This is a convenient method for VM. If VM calls getFileURL(File), it's still a new dependency. Is there any other way doing it? URLClassLoader.defineClass 438 * NOTE: the logic used here has been duplicated in the VM native code 439 * (search for invocation of definePackageInternal in the HotSpot sources). 440 * If this is changed, the VM code also need to be modified. Is the definePackageInternal only logic duplicated in the VM? I wonder if this comment can be clear what is duplicated. Mandy

Hi Ioi and Mandy, Please see comments below. On 08/11/2014 11:10 PM, Ioi Lam wrote:

On 8/11/14, 4:01 PM, Mandy Chung wrote:

...

On 8/11/14 2:15 PM, Ioi Lam wrote:

...

...

http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/

I would like to avoid adding private methods for VM to call as fewer as possible.

SecureClassLoader.getProtectionDomain(URL) Can you use the existing private getProtectionDomain(CodeSource) method instead? The VM can call the public CodeSource(URL, CodeSigner[]) constructor. I doubt an additional up call from the VM to Java may cause performance degradation as much while I think it's a good tradeoff. It also allows this be extended to signed JARs if it's considered.

Manifest.getManifest(byte[] buf) Is saving one additional upcall from VM to Java very critical? I prefer the VM to use public APIs to minimize such dependency in the library.

Launcher.getFileURL(String) This is a convenient method for VM. If VM calls getFileURL(File), it's still a new dependency. Is there any other way doing it?

Hi Mandy,

I agree we should avoid adding new private Java methods if possible. I've rewritten the 3 calls in C.

I also agree we should avoid adding new private Java methods if possible. Just some background why we added the Java code. These new Java methods are in performance critical path. Native->Java->Native->Java are expensive transitions. Doing the logic in C means we will have multiple expensive transitions in the code. Adding a private method and doing it in Java would also allows the JIT to compile the java code and further optimize the performance, while doing it in native prevents that. Thanks, Jiangli

It's a little messy: a single line of Java code needs to be written like:

// JAVA: CodeSource cs = new CodeSource(url, null);

InstanceKlass* cs_klass = InstanceKlass::cast(SystemDictionary::CodeSource_klass()); Handle cs = cs_klass->allocate_instance_handle(CHECK_(protection_domain)); { JavaValue result(T_VOID); JavaCalls::call_special(&result, cs, KlassHandle(THREAD, cs_klass), vmSymbols::object_initializer_name(), vmSymbols::url_code_signer_array_void_signature(), url, Handle(), CHECK_(protection_domain)); }

There's some minimal checking with argument types (default in debug build, -XX:+CheckJNICalls in product build), but I think there's no check if you forget to call the <init> method.

We could probably add a new JavaCalls::new_instance() function to make the code a little cleaner (and make sure the <init> is called). I can do this as a separate clean-up bug.

...

URLClassLoader.defineClass 438 * NOTE: the logic used here has been duplicated in the VM native code 439 * (search for invocation of definePackageInternal in the HotSpot sources). 440 * If this is changed, the VM code also need to be modified.

Is the definePackageInternal only logic duplicated in the VM? I wonder if this comment can be clear what is duplicated.

I'll reword it and send out a new webrev, along with the rest of the code.

Thanks - Ioi

...

Mandy

On 8/11/2014 11:23 PM, David Holmes wrote:

Hi Mandy,

On 12/08/2014 9:01 AM, Mandy Chung wrote:

...

On 8/11/14 2:15 PM, Ioi Lam wrote:

...

...

http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/

I would like to avoid adding private methods for VM to call as fewer as possible.

SecureClassLoader.getProtectionDomain(URL) Can you use the existing private getProtectionDomain(CodeSource) method instead? The VM can call the public CodeSource(URL, CodeSigner[]) constructor. I doubt an additional up call from the VM to Java may cause performance degradation as much while I think it's a good tradeoff. It also allows this be extended to signed JARs if it's considered.

An extra upcall per class loaded from the archive may indeed be a significant performance degradation!

Is it called per URL (jar file)? Same for manifest? Mandy

...

Manifest.getManifest(byte[] buf) Is saving one additional upcall from VM to Java very critical? I prefer the VM to use public APIs to minimize such dependency in the library.

Ditto.

David -----

...

Launcher.getFileURL(String) This is a convenient method for VM. If VM calls getFileURL(File), it's still a new dependency. Is there any other way doing it?

URLClassLoader.defineClass 438 * NOTE: the logic used here has been duplicated in the VM native code 439 * (search for invocation of definePackageInternal in the HotSpot sources). 440 * If this is changed, the VM code also need to be modified.

Is the definePackageInternal only logic duplicated in the VM? I wonder if this comment can be clear what is duplicated.

Mandy

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