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

[David Holmes]

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