RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Mikael,   see below.
On 6/24/14, 9:48 AM, Mikael Gerdin wrote:
> On Tuesday 24 June 2014 08.51.18 Coleen Phillimore wrote:
>> Hi Peter,
>>
>> On 6/24/14, 4:23 AM, Peter Levart wrote:
>>> On 06/24/2014 01:45 AM, Coleen Phillimore wrote:
>>>> Please review a change to the JDK code for adding classLoader field
>>>> to the instances of java/lang/Class.  This change restricts
>>>> reflection from changing access to the classLoader field.  In the
>>>> spec, AccessibleObject.setAccessible() may throw SecurityException if
>>>> the accessibility of an object may not be changed:
>>>>
>>>> http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObje
>>>> ct.html#setAccessible(boolean)
>>>>
>>>>
>>>> Only AccessibleObject.java has changes from the previous version of
>>>> this change.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>>>>
>>>> Thanks,
>>>> Coleen
>>> Hi Coleen,
>>>
>>> So hackers are prevented from hacking...
>>>
>>> Out of curiosity, what would happen if someone changed the classLoader
>>> field of some Class object? I guess VM still has it's own notion of
>>> the class' class loader, right? Only the code that directly uses the
>>> Class.getClassLoader() (and Unsafe.defineClass0) methods would be
>>> affected...
>> True, Class.getClassLoader() calls would be affected but we may use this
>> call for security checks.  I'm not really an expert on this, but we
>> thought it wouldn't be safe to allow user access to classLoader.
>>
>>> While we're at it, does this change in any way affect the GC logic?
>>> Will GC now navigate the classLoader field during marking but
>>> previously didn't ? Will this have any GC performance penalty ?
>> I actually ran this through our performance testing and got a good
>> improvement in one of the scimark benchmarks for no reason I could
>> explain (and lost the results), but none of the other tests were
>> affected.  GC would have to mark this new field for full collections but
>> not young collections because it's only set during initialization.   I
>> wouldn't expect this field to have any negative performance for GC.
> Peter has a good point actually. If the classLoader field is trustworthy we
> can get rid of some code in InstanceMirrorKlass::{oop_follow_contents,
> oop_oop_iterate} which currently picks up the class loader through the Klass*
> field injected into java/lang/Class. It would be a nice simplification if the
> ClassLoader objects were the only places where the GC would need to find the
> link to the ClassLoaderData metadata.
>
> But that won't work because anonymous (as in Unsafe.defineAnonymousClass)
> classes don't interact with class loaders in the same way and have their own
> ClassLoaderData objects.

Yes, one version of this patch removed class_loader_data from 
InstanceKlass so we'd be space-neutral but since there isn't a 1-1 
relationship to class_loader and ClassLoaderData for Unsafe anonymous 
classes, it doesn't work.  We might think of a way around this because 
we want to go in this direction.

Thanks,
Coleen
>
> /Mikael
>
>> Thanks,
>> Coleen
>>
>>> Regards, Peter
>>>
>>>> On 6/19/14, 11:01 PM, David Holmes wrote:
>>>>> On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:
>>>>>> Hi Mandy,
>>>>>>
>>>>>> On 19 jun 2014, at 22:34, Mandy Chung <mandy.chung at oracle.com> wrote:
>>>>>>> On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 19 jun 2014, at 20:46, Coleen Phillimore
>>>>>>>>
>>>>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>>>>> On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
>>>>>>>>>> Have you considered hiding the Class.classLoader field from
>>>>>>>>>> reflection? I’m not sure it is necessary but I’m not to keen on
>>>>>>>>>> the idea of people poking at this field with Unsafe (which
>>>>>>>>>> should go away in 9 but …).
>>>>>>>>> I don't know how to hide the field from reflection. It's a
>>>>>>>>> private final field so you need to get priviledges to make it
>>>>>>>>> setAccessible.  If you mean injecting it on the JVM side, the
>>>>>>>>> reason for this change is so that the JIT compilers can inline
>>>>>>>>> this call and not have to call into the JVM to get the class
>>>>>>>>> loader.
>>>>>>>> There is sun.reflect.Reflection.registerFieldsToFilter() that
>>>>>>>> hides a field from being found using reflection. It might very
>>>>>>>> well be the case that private and final is enough, I haven’t
>>>>>>>> thought this through 100%. On the other hand, is there a reason
>>>>>>>> to give users access through the field instead of having to use
>>>>>>>> Class.getClassLoader()?
>>>>>>> There are many getter methods that returns a private final field.
>>>>>>> I'm not sure if hiding the field is necessary nor a good precedence
>>>>>>> to set. Accessing a private field requires "accessDeclaredMember"
>>>>>>> permission although it's a different security check (vs
>>>>>>> "getClassLoader"
>>>>>>> permission).  Arguably before this new classLoader field, one could
>>>>>>> call Class.getClassLoader0() via reflection to get a hold of class
>>>>>>> loader.
>>>>>>>
>>>>>>> Perhaps you are concerned that the "accessDeclaredMember" permission
>>>>>>> is too coarse-grained?  I think the security team is looking into
>>>>>>> the improvement in that regards.
>>>>>> I think I’m a bit worried about two things, first as you wrote,
>>>>>> “accessDeclaredMember” isn’t the same as “getClassLoader”, but
>>>>>> since you could get the class loader through getClassLoader0() that
>>>>>> shouldn’t be a new issue.
>>>>>>
>>>>>> The second thing is that IIRC there are ways to set a final field
>>>>>> after initialization. I’m not sure we need to care about that
>>>>>> either if you need Unsafe to do it.
>>>>> Normal reflection can set a final field if you can successfully call
>>>>> setAccessible(true) on the Field object.
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> cheers
>>>>>> /Joel