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

[Jiangli Zhou]

Hi Ioi and Mandy,

Please see comments below.

On 08/11/2014 11:10 PM, Ioi Lam wrote:
> On 8/11/14, 4:01 PM, Mandy Chung wrote:
>> On 8/11/14 2:15 PM, Ioi Lam wrote:
>>
>>>> http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/
>>
>> I would like to avoid adding private methods for VM to call
>> as fewer as possible.
>>
>> SecureClassLoader.getProtectionDomain(URL)
>>    Can you use the existing private getProtectionDomain(CodeSource)
>>    method instead?  The VM can call the public CodeSource(URL, 
>> CodeSigner[])
>>    constructor.  I doubt an additional up call from the VM to Java
>>    may cause performance degradation as much while I think it's
>>    a good tradeoff.  It also allows this be extended to signed JARs
>>    if it's considered.
>>
>> Manifest.getManifest(byte[] buf)
>>    Is saving one additional upcall from VM to Java very critical?
>>    I prefer the VM to use public APIs to minimize such dependency
>>    in the library.
>>
>> Launcher.getFileURL(String)
>>    This is a convenient method for VM.  If VM calls getFileURL(File),
>>    it's still a new dependency.  Is there any other way doing it?
>>
> Hi Mandy,
>
> I agree we should avoid adding new private Java methods if possible. 
> I've rewritten the 3 calls in C. 

I also agree we should avoid adding new private Java methods if possible.

Just some background why we added the Java code. These new Java methods 
are in performance critical path. Native->Java->Native->Java are 
expensive transitions. Doing the logic in C means we will have multiple 
expensive transitions in the code. Adding a private method and doing it 
in Java would also allows the JIT to compile the java code and further 
optimize the performance, while doing it in native prevents that.

Thanks,
Jiangli

> It's a little messy: a single line of Java code needs to be written like:
>
>     // JAVA: CodeSource cs = new CodeSource(url, null);
>
>     InstanceKlass* cs_klass = 
> InstanceKlass::cast(SystemDictionary::CodeSource_klass());
>     Handle cs = 
> cs_klass->allocate_instance_handle(CHECK_(protection_domain));
>     {
>       JavaValue result(T_VOID);
>       JavaCalls::call_special(&result, cs, KlassHandle(THREAD, cs_klass),
> vmSymbols::object_initializer_name(),
> vmSymbols::url_code_signer_array_void_signature(),
>                               url, Handle(), CHECK_(protection_domain));
>     }
>
> There's some minimal checking with argument types (default in debug 
> build, -XX:+CheckJNICalls in product build), but I think there's no 
> check if you forget to call the <init> method.
>
> We could probably add a new JavaCalls::new_instance() function to make 
> the code a little cleaner (and make sure the <init> is called). I can 
> do this as a separate clean-up bug.
>> URLClassLoader.defineClass
>>    438      * NOTE: the logic used here has been duplicated in the VM 
>> native code
>>    439      * (search for invocation of definePackageInternal in the 
>> HotSpot sources).
>>    440      * If this is changed, the VM code also need to be modified.
>>
>>    Is the definePackageInternal only logic duplicated in the VM?
>>    I wonder if this comment can be clear what is duplicated.
>>
> I'll reword it and send out a new webrev, along with the rest of the 
> code.
>
> Thanks
> - Ioi
>> Mandy
>>
>