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

[Leela Mohan]

Mikael,

Found it during our internal code inspection and tracked it to this webrev
which got in the change.

Thanks,
Leela




On Fri, Jul 29, 2016 at 1:45 AM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:

> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>