Hi Dmitri, Thank you for the review. You're right, the code gets cleaner this way. New webrev: http://www.sapjvm.com/as/webrevs/8036666_3/ Thanks, Axel
On 17.03.2014 23:01, Dmitry Samersoff wrote: > Axel, > > The changes it self looks good for me. > > But it looks like the owning_thread is always NULL if > owner is NULL, so we can safely move this code > to ll.1017 and join two identical ifs together. > > Also comment on ll. 1019 is misleading, could you remove it? > > -Dmitry > > On 2014-03-13 12:19, Siebenborn, Axel wrote: >> 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 >>> >>> >> > >
