Hi Seguei, I still can't upload files to the cr.openjdk server. Meanwhile, I use our server for the new webrev:
http://www.sapjvm.com/as/webrevs/8036666/ Thanks, Axel Comments inline: On 11.03.2014 09:50, [email protected] wrote: > 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 I moved the test. > > 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? In order to have a synchronized method in the call chain, I synchronize on the "this" object. > 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? You're right! I moved to code up. > > 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 > I hope I fixed them all and added no new style violations. Do you have a tool to check this? > > 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, [email protected] wrote: >> Hi Axel, >> >> The webrev link does not work now. >> I'll check it again tomorrow. >> >> Thanks, >> Serguei >> >> On 3/7/14 1:32 AM, [email protected] wrote: >>> 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: >>>> >>>> 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: >>>> >>>> https://bugs.openjdk.java.net/browse/JDK-8036666 >>>> >>>> Webrev: >>>> >>>> http://cr.openjdk.java.net/~asiebenborn/8036666/webrev/ >>>> <http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/> >>>> >>>> Thanks, >>>> >>>> Axel >
