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

[Ioi Lam]

Hi David, thanks for the feedback. I will integrate your comments.

Everyone, due to upcoming deadlines, we would like to push this change 
into jdk9/hs-rt by this Thursday.

Please send additional comments, thumbs up/down by today if possible.

Thanks!

- Ioi

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 ??
>
> ---
>
> 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?
>
> ---
>
> 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?
>
> ---
>
> src/share/vm/runtime/javaCalls.cpp
>
> +     // may cause undesriable side-effect in the class metadata.
>
> Typo: undesriable; also side-effects
>
> ---
>
> 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
>>