On 7/08/2017 10:33 AM, Yasumasa Suenaga wrote:
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/
I have one concern with the test. Are you certain that the only possible
contended-enter event can come from the attempt to lock the
GetOwnedMonitorInfoTest.class object? Something from -Xcomp perhaps? The
test would likely still pass, but it would be checking the "wrong" event.
A few nits in the test
GetOwnedMonitorInfoTest.java:
Should have @bug 8185164
--
27 * @summary Verifies the JVMTI GetOwnedMonitorInfo API
That's a bit of an overstatement - this only verifies that a contended
monitor does not show up in the set of owned monitors. (Even that is
somewhat inaccurate)
--
Style-nit:
35 public class GetOwnedMonitorInfoTest implements Runnable {
There is no need to implement Runnable and provide the run() method as
an instance method of the class because there is no state. The simpler
construct would be to define an anonymous inner class when creating the
thread:
Thread t1 = new Thread() {
public void run() {
synchronized (GetOwnedMonitorInfoTest.class) {
System.out.println("Thread in sync section: " + name);
}
}
};
--
Style-nit:
42 System.err.println("java.library.path: "
43 + System.getProperty("java.library.path"));
line continuation should indent to align after ( i.e.
System.err.println("java.library.path: "
+ System.getProperty("java.library.path"));
--
Style-nit:
48 native static int check();
49 native static boolean hasEventPosted();
modifier order should be "static native". They could/should also be private.
--
Style-nit:
53 try {
54 synchronized (GetOwnedMonitorInfoTest.class) {
55 System.out.println("Thread in sync section: " + name);
56 }
57 } catch (Exception e) {
58 e.printStackTrace();
59 }
There is no need for a try-catch here as no Exceptions (other than
RuntimeExceptions) are possible, and the default exception handler will
take care of printing the stacktrace regardless.
--
Style-nit:
79 int status = check();
80 if (status != 0) {
No need for local variable as it's only used once.
---
libGetOwnedMonitorInfoTest.c
Style-nit:
50 static volatile jboolean event_has_posted = JNI_FALSE;
double-space after jboolean
--
Query:
64 fprintf(stderr, "MonitorContendedEnter: error in JVMTI
GetThreadInfo: %d\n", err);
do we not have anything to print out a more meaningful error message
that the integer value of the JVMTI error code?
--
Style query: do we do indent of 2 or 4 in hotspot C code tests ?
Thanks,
David
------
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
