Hi Dan, >> * GetOwnedMonitorInfoTest.java >> - I use Thread#yield() to wait until MonitorContendedEnter event is >> fired. > > > Please switch back to a small Thread.sleep(). Thread.yield() > can be a no-op on some platforms which will make the wait > loop very hot.
I fixed it in new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.03/ Thanks, Yasumasa 2017-08-06 23:54 GMT+09:00 Daniel D. Daugherty <daniel.daughe...@oracle.com>: > On 8/6/17 1:28 AM, Yasumasa Suenaga wrote: >> >> Hi Dan, Serguei, >> >> Thanks Serguei! I can run new testcase on my environment. >> >> Dan, I modified your patch and uploaded as webrev: >> >> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.02/ >> >> >> * GetOwnedMonitorInfoTest.java >> - I use Thread#yield() to wait until MonitorContendedEnter event is >> fired. > > > Please switch back to a small Thread.sleep(). Thread.yield() > can be a no-op on some platforms which will make the wait > loop very hot. > > >> * libGetOwnedMonitorInfoTest.c >> - I added "volatile" to "status" variable. > > > Sorry I missed that one. > > >> - I moved "event_has_posted = JNI_TRUE" to each before return >> statement. > > > Good catch. My original setting of event_has_posted had > a narrow race. > > I'm good on the changes. I'll have test results later today. > > Dan > > > >> >> >> Yasumasa >> >> >> On 2017/08/06 11:33, Daniel D. Daugherty wrote: >>> >>> Hi Yasumasa and Serguei, >>> >>> I've made some tweaks to the test and attached an updated patch: >>> >>> GetOwnedMonitorInfoTest.java changes: >>> >>> - deleted thread 't2' >>> - made changes to make monitor contention not racy: >>> - added 'hasEventPosted()' native function >>> - changed Main to grab the GetOwnedMonitorInfoTest.class monitor >>> before launching 't1' >>> - added loop to check hasEventPosted() function while holding >>> the GetOwnedMonitorInfoTest.class monitor >>> - moved the delay to this new loop >>> >>> libGetOwnedMonitorInfoTest.c changes: >>> >>> - added event_has_posted flag to know when MonitorContendedEnter >>> event has been posted >>> - added missing error checks to JVM/TI functions >>> - print an error message when MonitorContendedEnter or >>> MonitorContendedEntered gets an incorrect monitor count >>> - print error and warnings to 'stderr' >>> - add Java_GetOwnedMonitorInfoTest_hasEventPosted function >>> >>> I've tested the latest version of test in a repo without the fix >>> and it fails (as expected). Here's some sample output: >>> >>> ----------System.out:(7/231)---------- >>> Agent_OnLoad started >>> Agent_OnLoad finished >>> Main starting worker thread. >>> Main waiting for event. >>> Thread in sync section: Thread-1 >>> MonitorContendedEnter: Thread-1 owns 1 monitor(s) >>> MonitorContendedEntered: Thread-1 owns 1 monitor(s) >>> ----------System.err:(14/931)---------- >>> MonitorContendedEnter: FAIL: monitorCount should be zero. >>> java.lang.RuntimeException: FAILED status returned from the agent >>> at GetOwnedMonitorInfoTest.main(GetOwnedMonitorInfoTest.java:81) >>> at >>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native >>> Method) >>> at >>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) >>> at >>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >>> at java.base/java.lang.reflect.Method.invoke(Method.java:564) >>> at >>> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:115) >>> at java.base/java.lang.Thread.run(Thread.java:844) >>> >>> JavaTest Message: Test threw exception: java.lang.RuntimeException: >>> FAILED status returned from the agent >>> JavaTest Message: shutting down test >>> >>> >>> Dan >>> >>> >>> On 8/5/17 12:37 PM, serguei.spit...@oracle.com wrote: >>>> >>>> Hi Yasumasa, >>>> >>>> I use the following script to run the test on linux: >>>> >>>> % cat run.sh >>>> #!/bin/sh >>>> REPO=<my_repo> >>>> IMAGES=${REPO}/build/linux-x86_64-normal-server-release/images >>>> export JAVA_HOME=${IMAGES}/jdk >>>> export LD_LIBRARY_PATH=${IMAGES}/test/hotspot/jtreg/native >>>> >>>> jtreg -J-Dtest.java.opts='-server' \ >>>> -jdk ${JAVA_HOME} \ >>>> -nativepath:${LD_LIBRARY_PATH} \ >>>> $REPO/hotspot/test/serviceability/jvmti/GetOwnedMonitorInfo >>>> >>>> >>>> Thanks, >>>> Serguei >>>> >>>> >>>> On 8/5/17 06:31, Yasumasa Suenaga wrote: >>>>> >>>>> Hi Serguei, >>>>> >>>>> I uploaded new webrev: >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.01/ >>>>> >>>>> I tried to run serviceability/jvmti/GetOwnedMonitorInfo but it is >>>>> failed because jtreg needs -nativepath option. >>>>> But I didn't know what path should I set to -nativepath. >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>> On 2017/08/05 20:20, serguei.spit...@oracle.com wrote: >>>>>> >>>>>> Hi Yasumasa, >>>>>> >>>>>> Please, merge the converted testcase. >>>>>> It still might need more tweaks though. >>>>>> >>>>>> Thanks, >>>>>> Serguei >>>>>> >>>>>> >>>>>> On 8/5/17 03:18, Yasumasa Suenaga wrote: >>>>>>> >>>>>>> Hi Serguei, >>>>>>> >>>>>>> Thank you so much! >>>>>>> Should I merge your testcase? Or can you push this change? >>>>>>> >>>>>>> I agree to your change as a reviewer. >>>>>>> >>>>>>> >>>>>>> Yasumasa >>>>>>> >>>>>>> >>>>>>> On 2017/08/05 11:34, serguei.spit...@oracle.com wrote: >>>>>>>> >>>>>>>> The updated patch attached. >>>>>>>> Now the test is passed with the suggested fix and failed without it. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Serguei >>>>>>>> >>>>>>>> >>>>>>>> On 8/4/17 15:45, serguei.spit...@oracle.com wrote: >>>>>>>>> >>>>>>>>> On 8/4/17 14:26, Daniel D. Daugherty wrote: >>>>>>>>>> >>>>>>>>>> On 8/4/17 3:17 PM, serguei.spit...@oracle.com wrote: >>>>>>>>>>> >>>>>>>>>>> The patch is attached. >>>>>>>>>>> It may need some tweaks though. >>>>>>>>>>> I was not able to make it fail yet. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I don't think the original test had "failure" detection. >>>>>>>>>> You were just supposed to notice that a pending monitor >>>>>>>>>> was listed under the wrong list. >>>>>>>>> >>>>>>>>> Nothing is listed. >>>>>>>>> Strange thing is I do not see the monitor events fired. >>>>>>>>> I'm using 10 for testing. >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Serguei >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Dan >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 8/4/17 12:45, Daniel D. Daugherty wrote: >>>>>>>>>>>> >>>>>>>>>>>> Thanks Serguei! >>>>>>>>>>>> >>>>>>>>>>>> I happen to be doing a test run this weekend that includes most >>>>>>>>>>>> of the >>>>>>>>>>>> JPDA stack of tests so I'll include the following in my >>>>>>>>>>>> experiment: >>>>>>>>>>>> >>>>>>>>>>>> $ hg log -v -r tip >>>>>>>>>>>> changeset: 12872:bb66cd7c61b1 >>>>>>>>>>>> tag: 8185164.patch >>>>>>>>>>>> tag: qtip >>>>>>>>>>>> tag: tip >>>>>>>>>>>> user: dcubed >>>>>>>>>>>> date: Fri Aug 04 13:41:29 2017 -0600 >>>>>>>>>>>> files: src/share/vm/runtime/objectMonitor.cpp >>>>>>>>>>>> description: >>>>>>>>>>>> imported patch 8185164.patch >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> That will get the product code changes a complete round of >>>>>>>>>>>> testing >>>>>>>>>>>> on Solaris X64 at least... :-) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Great! >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Serguei >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Dan >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 8/4/17 1:31 PM, serguei.spit...@oracle.com wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Dan, >>>>>>>>>>>>> >>>>>>>>>>>>> Thank you for letting me know about this discussion. >>>>>>>>>>>>> I'll try to convert the attached test case to the JTreg format. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Serguei >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 8/4/17 11:16, Daniel D. Daugherty wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Adding Serguei to this thread directly since he's back from >>>>>>>>>>>>>> vacation! >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 7/31/17 10:14 PM, David Holmes wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi Dan, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 26/07/2017 11:52 PM, Daniel D. Daugherty wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 7/26/17 12:11 AM, David Holmes wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On 26/07/2017 10:27 AM, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hi Dan, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> I've added some analysis to the bug report >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thanks! >>>>>>>>>>>>>>>>>> I tried to fix this issue in >>>>>>>>>>>>>>>>>> JvmtiEnvBase::get_owned_monitors() at first. >>>>>>>>>>>>>>>>>> But it is difficult because we cannot know pending monitor >>>>>>>>>>>>>>>>>> if thread state is MONITOR_CONTENDED_ENTER when >>>>>>>>>>>>>>>>>> get_owned_monitor() is >>>>>>>>>>>>>>>>>> called. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I need to look closer at this when I get back from vacation >>>>>>>>>>>>>>>>> next week. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Seems like you're back already. :-) >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> A pending monitor should not be reported as owned (unless >>>>>>>>>>>>>>>>> the spec says otherwise) and it seems odd to me to fix the >>>>>>>>>>>>>>>>> current problem >>>>>>>>>>>>>>>>> by marking the monitor as pending earlier. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> It's the updating of the _current_pending_monitor field that >>>>>>>>>>>>>>>> allows JvmtiEnvBase::get_locked_objects_in_frame() to >>>>>>>>>>>>>>>> recognize >>>>>>>>>>>>>>>> that the monitor observed in the frame is only pending and >>>>>>>>>>>>>>>> is not owned. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I put a fairly detailed note in the bug yesterday, but you >>>>>>>>>>>>>>>> should look at that when you're officially back! >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks for clarifying things. I also added a comment to the >>>>>>>>>>>>>>> bug report. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I think the fix is sound and prevents anyone from observing >>>>>>>>>>>>>>> the case where the monitor will be seen in the stack-frame, but >>>>>>>>>>>>>>> has not yet >>>>>>>>>>>>>>> been set as the "pending monitor". As far as I can tell it is >>>>>>>>>>>>>>> only this case >>>>>>>>>>>>>>> (GetOwnedMonitorInfo from the contended-monitor event callback >>>>>>>>>>>>>>> in the >>>>>>>>>>>>>>> current thread) that will be able to observe the change. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> One scenario that I worry about here is that a >>>>>>>>>>>>>> GetCurrentContendedMonitor() >>>>>>>>>>>>>> call on a target thread will now be able to return a non-NULL >>>>>>>>>>>>>> value for the >>>>>>>>>>>>>> object, when GetThreadState() will be able to return something >>>>>>>>>>>>>> other than >>>>>>>>>>>>>> blocked (on monitor enter) for the thread. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I don't see anything in the JVM/TI spec that says such a >>>>>>>>>>>>>> scenario is >>>>>>>>>>>>>> wrong; I'm only worried about whether we have any tests that >>>>>>>>>>>>>> would catch >>>>>>>>>>>>>> this slight change in behavior. In any case, one of these >>>>>>>>>>>>>> operations has >>>>>>>>>>>>>> to "happen first": >>>>>>>>>>>>>> >>>>>>>>>>>>>> - thread is marked as blocked >>>>>>>>>>>>>> - monitor is flagged as contended >>>>>>>>>>>>>> >>>>>>>>>>>>>> Currently, they happen in the above order and the fix proposes >>>>>>>>>>>>>> to >>>>>>>>>>>>>> change the order and I see no reason not to do it. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I would like the test attached to the bug to be converted into >>>>>>>>>>>>>> a native >>>>>>>>>>>>>> JTREG test that lives in hotspot/test/serviceability/jvmti. >>>>>>>>>>>>>> See the >>>>>>>>>>>>>> following test as a possible example: >>>>>>>>>>>>>> >>>>>>>>>>>>>> hotspot/test/serviceability/jvmti/GetNamedModule >>>>>>>>>>>>>> >>>>>>>>>>>>>> for how to do this... I haven't done one of these new native >>>>>>>>>>>>>> JTREG >>>>>>>>>>>>>> tests myself, but I believe Serguei has... >>>>>>>>>>>>>> >>>>>>>>>>>>>> Dan >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>> David >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Dan >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>>> David >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Did you run the jdk repo's com/sun/jdi tests with your >>>>>>>>>>>>>>>>>>> fix? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I have not done yet. >>>>>>>>>>>>>>>>>> I have a trip until 28 July JST. So I will run it after >>>>>>>>>>>>>>>>>> that. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 2017/07/26 7:05, Daniel D. Daugherty wrote: >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On 7/24/17 8:40 PM, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> I tried to get owned monitors in MonitorContendedEnter >>>>>>>>>>>>>>>>>>>> JVMTI event handler. >>>>>>>>>>>>>>>>>>>> However GetOwnedMonitorInfo JVMTI function returns a >>>>>>>>>>>>>>>>>>>> monitor which is >>>>>>>>>>>>>>>>>>>> not yet owned. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> I attached reproducer to JBS. Please read README.md. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> I think GetOwnedMonitorInfo() should not return a >>>>>>>>>>>>>>>>>>>> pending monitor. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> I uploaded webrev. Could you review? >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.00/ >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> I hope this fix is applied to 8u or later release. >>>>>>>>>>>>>>>>>>>> I cannot access JPRT. So I need a sponsor. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Thanks for the bug report. It's nice to have a test case >>>>>>>>>>>>>>>>>>> and a proposed >>>>>>>>>>>>>>>>>>> fix all in the bug report! I've added some analysis to >>>>>>>>>>>>>>>>>>> the bug report >>>>>>>>>>>>>>>>>>> and we'll need to run this fix through Oracle's JPDA test >>>>>>>>>>>>>>>>>>> stack which >>>>>>>>>>>>>>>>>>> is not (yet) open. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Did you run the jdk repo's com/sun/jdi tests with your >>>>>>>>>>>>>>>>>>> fix? >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Dan >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>> >>> >
