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

List:       openjdk-serviceability-dev
Subject:    Re: RFR(MS): 8222072: JVMTI GenerateEvents() sends CompiledMethodLoad events to wrong jvmtiEnv
From:       Alex Menkov <alexey.menkov () oracle ! com>
Date:       2019-04-10 16:53:06
Message-ID: 8f4b2574-d589-f851-3184-9eb1140bc3a0 () oracle ! com
[Download RAW message or body]

Much simpler and cleaner!
LGTM.

--alex

On 04/09/2019 18:56, serguei.spitsyn@oracle.com wrote:
> Hi Alex,
> 
> New webrev version with refactoring:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.3/ 
> 
> 
> I will also re-submit mach5 jobs for jvmti tests and new test.
> 
> Thanks,
> Serguei
> 
> 
> On 4/9/19 5:39 PM, serguei.spitsyn@oracle.com wrote:
> > Hi Alex,
> > 
> > Thank you for review!
> > 
> > On 4/9/19 4:57 PM, Alex Menkov wrote:
> > > Hi Serguei,
> > > 
> > > Maybe resolve code duplication in post_compiled_method_load(nmethod 
> > > *nm) and post_compiled_method_load(JvmtiEnv* env, nmethod *nm)?
> > > 
> > > Looks like the only difference is logging, but I don't think it's 
> > > important as old post_compiled_method_load(JvmtiEnv* env, const 
> > > jmethodID method, const jint length, ...) was not used.
> > 
> > Another difference is that in case of GenerateEvents the phase is 
> > guaranteed to be LIVE.
> > So, there is no need for the PRINORDIAL phase check.
> > 
> > Also, I'm not sure yet how important the difference in logging is.
> > Let me think a little bit more on this.
> > 
> > Thanks,
> > Serguei
> > 
> > > --alex
> > > 
> > > On 04/09/2019 13:58, serguei.spitsyn@oracle.com wrote:
> > > > Hi Jc,
> > > > 
> > > > Thank you a lot for looking at this!
> > > > 
> > > > On 4/9/19 9:29 AM, Jean Christophe Beyler wrote:
> > > > > Hi Serguei,
> > > > > 
> > > > > I saw a nit here:
> > > > > 
> > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEven \
> > > > > ts.1/test/hotspot/jtreg/serviceability/jvmti/GenerateEvents/MyPackage/GenerateEventsTest.java.html \
> > > > >  <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8222072-jvmti-Generate \
> > > > > Events.1/test/hotspot/jtreg/serviceability/jvmti/GenerateEvents/MyPackage/GenerateEventsTest.java.html> \
> > > > >  
> > > > > 
> > > > > arei -> are
> > > > 
> > > > Nice catch, fixed.
> > > > 
> > > > > - I'm not sure you needed two files for the agents, you could have 
> > > > > re-used some of the code and just called twice GetEnv but that is a 
> > > > > detail.
> > > > 
> > > > Right.
> > > > I initially wanted to use this way but then decided to use two real 
> > > > agent libraries.
> > > > Wanted to exercise this path.
> > > > 
> > > > > - I thought JNIEnv was not supposed to be really kept because it 
> > > > > should not be used by another thread, is there not a risk that you 
> > > > > are doing that? It doesn't look like it but I've been surprised in 
> > > > > the past
> > > > 
> > > > You are right.
> > > > Tried to fix it in new webrev version below.
> > > > 
> > > > 
> > > > > - Isn't there a chance that your second agent gets a normal 
> > > > > JVMTI_EVENT_COMPILED_METHOD_LOAD before the GenerateEvents call and 
> > > > > increments its counter?
> > > > > - I guess we'd see if it becomes flaky at some point? :)
> > > > Yes, it is expected.
> > > > But they should be posted on threads other than Main thread.
> > > > The callback should ignore them.
> > > > 
> > > > 
> > > > The updated fix version is:
> > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.2/ \
> > > >  
> > > > 
> > > > 
> > > > Thanks!
> > > > Serguei
> > > > 
> > > > > 
> > > > > Thanks!
> > > > > Jc
> > > > > 
> > > > > 
> > > > > On Mon, Apr 8, 2019 at 12:29 PM serguei.spitsyn@oracle.com 
> > > > > <mailto:serguei.spitsyn@oracle.com> <serguei.spitsyn@oracle.com 
> > > > > <mailto:serguei.spitsyn@oracle.com>> wrote:
> > > > > 
> > > > > Please, review a fix for:
> > > > > https://bugs.openjdk.java.net/browse/JDK-8222072
> > > > > 
> > > > > Webrev:
> > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.1/ \
> > > > >  
> > > > > <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.1/> \
> > > > >  
> > > > > 
> > > > > 
> > > > > Summary:
> > > > > The JVMTI GenerateEvents() must send CompiledMethodLoad events
> > > > > only to the agent which called it.
> > > > > However, it sends events to all agents (jvmti environements)
> > > > > which violates the JVMTI spec.
> > > > > The webrev above fixes this issue and adds new jtreg test:
> > > > > test/hotspot/jtreg/serviceability/jvmti/GenerateEvents
> > > > > 
> > > > > 
> > > > > Testing:
> > > > > Mach5 submission for:
> > > > > - JVMTI tests: open/test/hotspot/jtreg/vmTestbase/nsk/jvmti
> > > > > - new test: test/hotspot/jtreg/serviceability/jvmti/GenerateEvents
> > > > > 
> > > > > Thanks,
> > > > > Serguei
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > 
> > > > > Thanks,
> > > > > Jc
> > > > 
> > 
> 


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