RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #3
David Holmes
david.holmes at oracle.com
Tue Aug 12 07:09:54 UTC 2014
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
>>>
>
More information about the core-libs-dev
mailing list