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

[Ioi Lam]

On 8/7/14, 5:15 PM, Coleen Phillimore wrote:
>
> Hi Ioi and team,
>
> Here are a couple more comments about this CDS cleanup.
>
> void MetaspaceShared::check_one_shared_class and 
> MetaspaceShared::link_on_shared_class
>
> belong in InstanceKlass, which has knowledge of the interface and 
> other fields inside of InstanceKlass.  It looks like the code was 
> written using code in InstanceKlass.cpp so these should be together.
>
I moved MetaspaceShared::check_one_shared_class to InstanceKlass.cpp. I 
left link_one_shared_class as-is since it's a wrapper to call 
MetaspaceShared::try_link_class(), which doesn't seem to belong to 
InstanceKlass.cpp

Thanks
- Ioi
> That's all I have comments on.  This looks good.
>
> Coleen
>
> On 8/1/14, 6:29 PM, Coleen Phillimore wrote:
>>
>> Ioi and team,   Here is a partial review.
>>
>> Can you recode this:
>>
>> +    // Used by the VM to create the ProtectionDomain for the url.
>> +    private ProtectionDomain getProtectionDomain(URL url) {
>> +        CodeSource cs = null;
>> +        CodeSigner[] signers = null;
>> +        cs = new CodeSource(url, signers);
>> +
>> +        return getProtectionDomain(cs);
>> +    }
>> +
>>
>> As
>>
>> +    // Used by the VM to create the ProtectionDomain for the url.
>> +    private ProtectionDomain getProtectionDomain(URL url) {
>> +        CodeSigner[] signers = null;
>> +        CodeSource cs = new CodeSource(url, signers);
>> +
>> +        return getProtectionDomain(cs);
>> +    }
>> +
>>
>> The variable signers is probably useful for documentation rather than 
>> passing null.
>>
>> For hotspot changes, this is partial:
>>
>> in instanceKlass.hpp how can you reuse the Breakpoints field?
>>
>> +  // CDS uses the field to record the ClassPathEntry where the class 
>> was
>> +  // was loaded. For archived classes, this filed is either NULL 
>> (for system
>> +  // classes), or points to a ClassPathEntry in the archive. When a 
>> class is
>> +  // being loaded from the archive, the VM uses the ClassPathEntry 
>> to restore
>> +  // the class ProtectionDomain. The ClassPathEntry information is 
>> no longer
>> +  // needed after afterwards.
>> +  union {
>>    BreakpointInfo* _breakpoints;          // bpt lists, managed by 
>> Method*
>> +    int shared_class_path_index;
>> +  } _info;
>>
>>
>> in arguments.cpp, can you add a general comment to fix_appclasspath() 
>> so one has to only read this C code once? What is it fixing it to 
>> do?  Is there some strstr C runtime function you can call instead of 
>> p++, memmove, etc?
>>
>> In dictionary.cpp
>>
>> +        ResourceMark rm;
>> +        tty->print_cr("Removed error class: %s", ik->external_name());
>>
>> This should be under some PrintSharedSpaces flag.
>>
>> There are a LOT of new flags.  Do we really need all of these flags???
>>
>> in vmStructs.cpp
>>
>> Since the breakpoints field is changed, there may be associated 
>> changes in the serviceability agent.
>>
>> I haven't code reviewed the new files yet.   Monday.
>>
>> Thanks,
>> Coleen
>>
>> On 7/28/14, 7: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
>>
>