Hi Serguei, I created a new webrev: http://www.sapjvm.com/as/webrevs/8036666_1/
I incorporated your suggestions and moved the test files. Thanks, Axel On 11.03.2014 20:25, [email protected] wrote: > On 3/11/14 12:05 PM, Staffan Larsen wrote: >> >> On 11 mar 2014, at 16:48, Siebenborn, Axel <[email protected] >> <mailto:[email protected]>> wrote: >>> 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] >>> <mailto:[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. >> >> Tests should avoid the bug number in the name or path and instead use a >> descriptive name. See this page for some background: >> https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests > > 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 >> >> Thanks, >> /Staffan >>>> >>>> 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] >>>> <mailto:[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] >>>>> <mailto:[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/><http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Axel >
