RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #3

[Ioi Lam]

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