On 11 mar 2014, at 16:48, Siebenborn, Axel <[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] 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 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] 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
