<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Axel,<br>
<br>
Thank you for the changes!<br>
It looks good, but one more place need a fix (expected must be 4
now):<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre> 230 if (recursionCount != 4) {
231 throw new AssertionError("recursions: expected 3, but was " + recursionCount);
232 }</pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 3/12/14 8:21 AM, Siebenborn, Axel wrote:<br>
</div>
<blockquote
cite="mid:02D5D45C1F8DB848A7AE20E80EE61A5C39812EC9@DEWDFEMB20C.global.corp.sap"
type="cite">
<pre wrap="">Hi Serguei,
I created a new webrev:
<a class="moz-txt-link-freetext" href="http://www.sapjvm.com/as/webrevs/8036666_1/">http://www.sapjvm.com/as/webrevs/8036666_1/</a>
I incorporated your suggestions and moved the test files.
Thanks,
Axel
On 11.03.2014 20:25, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 3/11/14 12:05 PM, Staffan Larsen wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
On 11 mar 2014, at 16:48, Siebenborn, Axel <<a class="moz-txt-link-abbreviated" href="mailto:axel.siebenborn@sap.com">axel.siebenborn@sap.com</a> <a class="moz-txt-link-rfc2396E" href="mailto:axel.siebenborn@sap.com"><mailto:axel.siebenborn@sap.com></a>> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Seguei,
I still can't upload files to the cr.openjdk server.
Meanwhile, I use our server for the new webrev:
<a class="moz-txt-link-freetext" href="http://www.sapjvm.com/as/webrevs/8036666/">http://www.sapjvm.com/as/webrevs/8036666/</a>
Thanks,
Axel
Comments inline:
On 11.03.2014 09:50,<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Axel,
The webrev link is resolvable now.
Thank you for taking care about your broken account on the cr.openjdk server!
I have some comments on the test case ...
- This is nice test, thank you for providing it!
- The location of the test must be different as it is a JVMTI test:
test/serviceability/jvmti/8036666 instead of test/runtime/8036666
</pre>
</blockquote>
<pre wrap="">
I moved the test.
</pre>
</blockquote>
<pre wrap="">
Tests should avoid the bug number in the name or path and instead use a descriptive name. See this page for some background: <a class="moz-txt-link-freetext" href="https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests">https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests</a>
</pre>
</blockquote>
<pre wrap="">
The test files have already descriptive names.
So, it would be enough to remove the bug number from the path.
Sorry, I had to catch it too in the first place.
Thanks,
Serguei
</pre>
<blockquote type="cite">
<pre wrap="">
Thanks,
/Staffan
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">
RecursiveObjectLock,java:
- A suggestion to add a synchronized method (say, nestedLock3) into the chain
of calls started from the testMethod. In order to do so, the class RecursiveObjectLock
needs to extend the ALock class. And the this object needs to be used in the
synchronized statements and for wait()?
What do you think about such test enhancement for better coverage?
</pre>
</blockquote>
<pre wrap="">
In order to have a synchronized method in the call chain, I synchronize on the "this" object.
</pre>
<blockquote type="cite">
<pre wrap="">GetObjectLockCount.java:
- The comment line 283 seems to be obsolete as the "param out" is not present anymore:
283 * @param out Stream to copy to
- Could you, please, add e.printStackTrace() into the catch statements at the lines 232 and 300?
- A question:
It seems the errThread and outThread are started a little bit late.
Would it be better to start them earlier, or it was intentional?
</pre>
</blockquote>
<pre wrap="">
You're right! I moved to code up.
</pre>
<blockquote type="cite">
<pre wrap="">
Some minor style-related comments (I hope, it is easy to fix this before push):
- Unneeded extra empty lines: 149, 174-175, 244
- A space is missed before the '{': 180, 242, 243, 246
- Unneeded extra space after and before the "(": 235, 297
- The curly brackets '{' do not follow the common style: 142, 144
</pre>
</blockquote>
<pre wrap="">
I hope I fixed them all and added no new style violations.
Do you have a tool to check this?
</pre>
<blockquote type="cite">
<pre wrap="">
We still need another reviewer for this fix.
I'm ready to be a sponsor for it.
Thanks,
Serguei
On 3/10/14 12:00 AM, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Axel,
The webrev link does not work now.
I'll check it again tomorrow.
Thanks,
Serguei
On 3/7/14 1:32 AM, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Axel,
Thank you for fixing this issue.
I'm reviewing it.
It looks good in general, but a little bit more time is needed to look at the tests.
Do you need a sponsor for pushing?
Thanks,
Serguei
On 3/6/14 12:08 AM, Siebenborn, Axel wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
Hi all,
could I have a review for the following change?
The recursive lock count for an object is not correct, in cases, where a monitor is inflated after recursive lightweight locks. In this case, the recursion count is taken from the heavyweight monitor, represented by the class ObjectMonitor. ObjectMonitor::_recursions is the number of times ObjectMonitor::enter() was called to acquire the lock minus 1. This counter does not include the recursions of lightweight locks, that happen before inflating the monitor and is not equal to the recursion count from a Java source level point of view.
I added a test to the webrev to reproduce the problem.
The suggested fix is to call count_locked_objects, even if there's a heavyweight monitor and get the recursion count by iterating the vframes.
Bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8036666">https://bugs.openjdk.java.net/browse/JDK-8036666</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~asiebenborn/8036666/webrev/">http://cr.openjdk.java.net/~asiebenborn/8036666/webrev/</a> <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/"><http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/></a><a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/"><http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/></a>
Thanks,
Axel
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
</blockquote>
<br>
</body>
</html>