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

[Coleen Phillimore]

Hi Ioi and team,

Here are a couple more comments about this CDS cleanup.

voidMetaspaceShared::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.

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
>