[8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[Mikael Gerdin]

Leela,
Thanks for the report!
I filed https://bugs.openjdk.java.net/browse/JDK-8162766 to track the issue.

I must ask, how did you find this in that old 2014 webrev?

/Mikael

On 2016-07-29 10:35, Mikael Gerdin wrote:
> Hi,
>
> (dropping core-libs since this is a VM-specific issue)
>
> On 2016-07-29 09:55, Leela Mohan wrote:
>> Hi David,
>>
>> I understand, Klass types are no longer oops  but
>> JNIHandles::resolve_non_null()
>> would expose naked oops. In other words, KlassOops are no longer oops but
>> java.lang.Class objects are.
>
> I agree.
> In this case it's not only the java.lang.Class but also the ClassLoader
> oop.
> In an unrelated change I've toyed with adding an assertion that the
> current thread is not _thread_in_native in JNIHandles::resolve.. to
> catch similar problems.
>
> The code in the webrev explicitly transitions to native and as such
> there can be a GC in progress when get_class_loader is being executed
> and while resolving the handles and creating new ones are probably not
> going to crash we could be creating an invalid handle at line 962.
>
> I'm unable to locate the corresponding code in JDK 9 but in 8u the
> potential problem persists.
>
> The 8u UnsafeDefineClass0 needs the native transition because the JVM_
> entrypoints expect it but all of them just transition back to _in_vm so
> if the code could just stay in the VM and use internal VM APIs to get
> the caller class and protection domains this would of course not be an
> issue.
>
>
> /Mikael
>
>>
>> Thanks,
>> Leela
>>
>> On Thu, Jul 28, 2016 at 10:51 PM, David Holmes <david.holmes at oracle.com>
>> wrote:
>>
>>> Hi Leela,
>>>
>>> On 29/07/2016 12:59 PM, Leela Mohan wrote:
>>>
>>>> I think, change in the file unsafe.cpp is incorrect. (
>>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )
>>>>
>>>> Below function is accessing naked oops when thread has transitioned to
>>>> "native":
>>>>
>>>> *+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
>>>> (java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
>>>> {**+     return NULL;**+   }**+   Klass* k =
>>>> java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
>>>> loader = k->class_loader();**+   return JNIHandles::make_local(env,
>>>> loader);**+ }*
>>>>
>>>
>>> klass types are no longer oops in the Java heap, but metaspace objects
>>> that reside in a per-classloader allocation region. They never get
>>> compacted or relocated so raw pointers to them are safe.
>>>
>>> Cheers,
>>> David
>>>
>>>
>>>
>>>> - Leela
>>>>
>>>> On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
>>>> coleen.phillimore at oracle.com> wrote:
>>>>
>>>>
>>>>> Thanks, Mandy!
>>>>>
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 9/8/14, 6:59 PM, Mandy Chung wrote:
>>>>>
>>>>> Thumbs up.
>>>>>>
>>>>>> Mandy
>>>>>>
>>>>>> On 9/5/2014 12:55 PM, Coleen Phillimore wrote:
>>>>>>
>>>>>> Summary: Add classLoader to java/lang/Class instance for fast access
>>>>>>>
>>>>>>> This is a backport request for 8u40.   This change has been in
>>>>>>> the jdk9
>>>>>>> code for 3 months without any problems.
>>>>>>>
>>>>>>> The JDK changes hg imported cleanly.  The Hotspot change needed a
>>>>>>> hand
>>>>>>> merge for create_mirror call in klass.cpp.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
>>>>>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/
>>>>>>>
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>>>>>>>
>>>>>>> Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes.
>>>>>>> Also ran
>>>>>>> jck java_lang tests with only the hotspot change.  The hotspot
>>>>>>> change
>>>>>>> can
>>>>>>> be tested separately from the jdk change (but not the other way
>>>>>>> around).
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>