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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR: 8038332: The trace event vm/class/load is not always being sent
From:       Karen Kinnear <karen.kinnear () oracle ! com>
Date:       2016-06-27 19:46:38
Message-ID: 4A995A5C-C11E-4277-81A0-AB48876A5B12 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic