Hi Serguei, new webrev: http://www.sapjvm.com/as/webrevs/8036666_2/ I should review my own changes more carefully. Sorry for that. Thanks, Axel
On 12.03.2014 18:34, [email protected] wrote: > Hi Axel, > > Thank you for the changes! It looks good, but one more place need a > fix (expected must be 4 now): > > 230 if (recursionCount != 4) { 231 throw new > AssertionError("recursions: expected 3, but was " + recursionCount); > 232 } > > > Thanks, Serguei > > > On 3/12/14 8:21 AM, Siebenborn, Axel wrote: >> 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 > >
