RFR: 8038332: The trace event vm/class/load is not always being sent

[Karen Kinnear]

I asked Max to double check if we are getting two JFR events for code that uses resolve_instance_class_or_null
and then calls resolve_from_stream when called by the class loader as part of JVM_DefineClass, which I
believe is the common case for loading classes.

I thought it made sense to record this where we post the jvmti load class event - and then remembered why
Calvin and I didn’t do that to start with - with jvmti you would get a load class event for the defining loader and
another one for the initiating loader, i.e. two events, each with a single class loader. Erik wanted to get a single event 
that contained both loaders, which is why we put this in resolve_instance_class_or_null. So today, this records
vm class resolution.

But that does miss the direct callers of loadClass (JVM_DefineClass), JNI_DefineClass and UnsafeDefineClass.

I am still trying to figure out a clean and not to slow way to determine if we are already in resolve_instance_class_or_null.
We do have a placeholder table entry, but it has a lookup by name/initiating class loader, and at JVM_DefineClass time
we don’t know the initiating loader. Creative solutions welcome - my guess is we may have cases in which we print twice,
even with creative filtering, but I would prefer to not have that be the common case.

thanks,
Karen

> On Jun 23, 2016, at 5:37 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Agreed! Good to use consistent code style in these related functions.
> 
> Thanks,
> David
> 
> On 24/06/2016 5:32 AM, Coleen Phillimore wrote:
>> 
>> This looks so much better.
>> Thanks!
>> Coleen
>> 
>> On 6/23/16 2:28 PM, Max Ockner wrote:
>>> Hello again!
>>> 
>>> New webrev: http://cr.openjdk.java.net/~mockner/8038332.03/
>>> 
>>> Thanks,
>>> Max
>>> 
>>> 
>>> On 6/22/2016 10:53 PM, Coleen Phillimore wrote:
>>>> 
>>>> 
>>>> On 6/22/16 5:02 PM, Ioi Lam wrote:
>>>>> Instead of using Exceptions::_throw_msg directly, I think it's
>>>>> better to use the macro
>>>>> 
>>>>>    THROW_MSG_NULL(vmSymbols::java_lang_SecurityException(), message);
>>>>> 
>>>>> It will do the "return NULL" for you, so there's no danger of
>>>>> forgetting doing it.
>>>> 
>>> Done.
>>> 
>>>> Agree.
>>>> Coleen
>>>>> 
>>>>> Thanks
>>>>> - Ioi
>>>>> 
>>>>> On 6/22/16 12:13 PM, Coleen Phillimore wrote:
>>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>> Can I suggest actually fixing this code?  Please?
>>>>>> 
>>>>>> Instead of returning THREAD in all the calls in parse_stream(),
>>>>>> they should have CHECK_NULL.   Then all the if
>>>>>> (!HAS_PENDING_EXCEPTION) conditionals can be removed and the logic
>>>>>> fixed.
>>>>>> 
>>>>>> The only reason to have THREAD as the last parameter is if there's
>>>>>> cleanup that needs to be done in the case of an exception, which is
>>>>>> rare and I verified not the case in this function.
>>>>>> 
>>>>>> After this code put a return NULL;
>>>>>> 
>>>>>>    Exceptions::_throw_msg(THREAD_AND_LOCATION,
>>>>>>      vmSymbols::java_lang_SecurityException(), message);
>>>>>> //  add return NULL;
>>>>>>  }
>>>>>> 
>>>>>> Otherwise, the code to add the event looks good.
>>>>>> 
>>>>>> Thanks,
>>>>>> Coleen
>>>>>> 
>>>>>> 
>>>>>> On 6/22/16 1:53 PM, Jiangli Zhou wrote:
>>>>>>> Hi Max,
>>>>>>> 
>>>>>>> I see there is already an ‘if (!HAS_PENDING_EXCEPTION)’ check at
>>>>>>> line 1170 before the debug_only code. Maybe you cloud place the
>>>>>>> post_class_load_event() in there so it only post the event when
>>>>>>> there is no pending exception. That way you don’t need to change
>>>>>>> the existing logic and add the additional checks.
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>> 
>>>>>>>> On Jun 20, 2016, at 12:08 PM, Max Ockner <max.ockner at oracle.com>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> David,
>>>>>>>> 
>>>>>>>> New webrev:
>>>>>>>> http://cr.openjdk.java.net/~mockner/8038332.02/src/share/vm/classfile/systemDictionary.cpp.cdiff.html
>>>>>>>> 
>>>>>>>> I have added the check you suggested before triggering the event:
>>>>>>>> 
>>>>>>>> if (HAS_PENDING_EXCEPTION || k.is_null()) {
>>>>>>>>    return NULL;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Max
>>>>>>>> 
>>>>>>>> On 6/13/2016 6:40 PM, David Holmes wrote:
>>>>>>>>> Hi Max,
>>>>>>>>> 
>>>>>>>>> On 14/06/2016 3:42 AM, Max Ockner wrote:
>>>>>>>>>> Jiangli,
>>>>>>>>>> Thanks for looking.  I didn't see anything that looked like it
>>>>>>>>>> might
>>>>>>>>>> produce duplicate events. However, I did see some additional
>>>>>>>>>> places
>>>>>>>>>> where it looks like no event is fire.
>>>>>>>>>> 
>>>>>>>>>> Can anyone point me to the event specs?
>>>>>>>>> I'm not sure there is any spec for this. Even JFR doesn't seem
>>>>>>>>> to document individual events and when they are triggered.
>>>>>>>>> 
>>>>>>>>> Your change does not look right however as you are posting the
>>>>>>>>> classload event regardless of the exception state. If you look
>>>>>>>>> at SystemDictionary::resolve_instance_class_or_null, it only
>>>>>>>>> posts after checking the load was successful:
>>>>>>>>> 
>>>>>>>>> 892   if (HAS_PENDING_EXCEPTION || k.is_null()) {
>>>>>>>>> 893     return NULL;
>>>>>>>>> 894   }
>>>>>>>>> 895
>>>>>>>>> 896   post_class_load_event(class_load_start_time, k,
>>>>>>>>> class_loader);
>>>>>>>>> 
>>>>>>>>> Similarly, SystemDictionary::parse_stream, has various CHECK
>>>>>>>>> macros that will cause a return on exception, prior to getting
>>>>>>>>> to the point of posting the load event. So you also need to add
>>>>>>>>> a check for a pending exception and that k is not null, I think,
>>>>>>>>> before posting the event.
>>>>>>>>> 
>>>>>>>> I have added this check.
>>>>>>>>> BTW I would have expected to see trace-events generated at
>>>>>>>>> approximately the same locations as the corresponding JVMTI
>>>>>>>>> events. That does not seem to be the case which seems very
>>>>>>>>> strange to me. The notion of "loading a class" seems to be
>>>>>>>>> spread across far too many functions to me.
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Max
>>>>>>>>>> 
>>>>>>>>>> On 6/9/2016 4:39 PM, Jiangli Zhou wrote:
>>>>>>>>>>> Hi Max,
>>>>>>>>>>> 
>>>>>>>>>>> Looks ok. The only possible issue is more than one event might
>>>>>>>>>>> be sent
>>>>>>>>>>> in some of the call paths. But my quick search didn’t find any
>>>>>>>>>>> of such
>>>>>>>>>>> case.
>>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Jiangli
>>>>>>>>>>> 
>>>>>>>>>>>> On Jun 9, 2016, at 11:05 AM, Max Ockner
>>>>>>>>>>>> <max.ockner at oracle.com> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> Hello,
>>>>>>>>>>>> 
>>>>>>>>>>>> Please review this small fix which causes the vm/class/load
>>>>>>>>>>>> event to
>>>>>>>>>>>> be fired from JVM_DefineClass() and JVM_DefineClassWithSource().
>>>>>>>>>>>> 
>>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~mockner/8038332/
>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8038332
>>>>>>>>>>>> 
>>>>>>>>>>>> The vm/class/load event  (EventClassLoad) was previously
>>>>>>>>>>>> fired in 2
>>>>>>>>>>>> places:
>>>>>>>>>>>> 
>>>>>>>>>>>> SystemDictionary::parse_stream
>>>>>>>>>>>> SystemDictionary::resolve_instance_class_or_null
>>>>>>>>>>>> 
>>>>>>>>>>>> parse_stream is the standard option for creating a klass from a
>>>>>>>>>>>> stream, but JVM_DefineClass uses a different function:
>>>>>>>>>>>> 
>>>>>>>>>>>> SystemDictionary::resolve_from_stream.
>>>>>>>>>>>> 
>>>>>>>>>>>> This did not fire a vm/class/load event. Now it does fire the
>>>>>>>>>>>> event.
>>>>>>>>>>>> 
>>>>>>>>>>>> Sanity tested with jtreg runtime. Issue was reproduced and
>>>>>>>>>>>> tested
>>>>>>>>>>>> using the reproducer script attached to the bug
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Max
>>>>>> 
>>>>> 
>>>> 
>>> 
>>