Thank you, David. Regards, Jini.
> -----Original Message----- > From: David Holmes > Sent: Wednesday, July 20, 2016 1:01 PM > To: Serguei Spitsyn; Jini George; [email protected] > Subject: Re: PING: RFR: JDK-8145627 > (sun.jvm.hotspot.oops.InstanceKlass::getSize() returns the incorrect > size and has no test) > > Just to clarify something ... > > On 20/07/2016 5:22 PM, [email protected] wrote: > > Jini, > > > > The fix looks good to me. > > Thank you for the update! > > > > > > Could you fix a couple of nits, please? > > > > test/serviceability/sa/TestInstanceKlassSize.java > > > > > > 156 agent.attach( (int) pid); > > > > > > Do not need to cast to int and the space before pid is not needed. > > > > The lines 114 -120 need standard indentation (4). (I'd suggest to > move > > '{' to the line 114). > > Something similar to the lines 106-113. (it'd probably makes sense > to > > align the '}' with the 'new') > > > > > > test/serviceability/sa/TestInstanceKlassSizeForInterface.java > > > > > > > > 70 agent.attach( (int) pid); > > > > > > The same as above. > > > > > > 162 if ( args == null || args.length == 0 ) { > > > > > > Unneeded spaces in condition. > > Those being the spaces after the (, and before the ). We use spaces > around operators. > > Cheers, > David > ----- > > > Lines 109-116 and 151-154 - the same comment as for prev. file > above. > > > > > > I don't need to see another webrev. > > > > > > Thanks, > > Serguei > > > > > > > > > > On 7/18/16 22:12, Jini George wrote: > >> > >> Thank you, Serguei, for the review. > >> > >> > >> > >> You are right, getBytesPerWord() makes more sense. Hence I modified > >> the code to use getBytesPerWord(), though both those > (getAddressSize() > >> and getBytesPerWord()) seem to return the same value. I have > retained > >> the call to alignSize() since the header size is not multiplied by > >> word size. > >> > >> > >> > >> I have addressed the comments related to the test cases. Here is a > >> modified webrev: > >> > >> > >> > >> > <http://cr.openjdk.java.net/%7Esballal/sponsorship/8145627/webrev.01/>h > ttp://cr.openjdk.java.net/~sballal/sponsorship/8145627/webrev.01/ > >> > >> > >> > >> Would you please take a relook ? > >> > >> > >> > >> Thank you, > >> > >> Jini. > >> > >> > >> > >> > >> > >> *From:*Serguei Spitsyn > >> *Sent:* Friday, July 15, 2016 3:21 PM > >> *To:* Jini George; [email protected] > >> *Subject:* Re: PING: RFR: JDK-8145627 > >> (sun.jvm.hotspot.oops.InstanceKlass::getSize() returns the incorrect > >> size and has no test) > >> > >> > >> > >> Hi Jini, > >> > >> Some questions. > >> > >> Is the call of the method alignSize(size) necessary? > >> It seems that the size is already aligned by the way it is > calculated > >> as the number of words is multiplied to wordLength. > >> But I'm not exactly sure because the wordLength is returned by > >> VM.getVM().getAddressSize() > >> but the VM.getVM().getBytesPerWord() is used in the alignSize:* > >> > >> public static long *alignSize(*long *size) { > >> // natural word size/ > >> /*return *VM.getVM().alignUp(size, VM.getVM().getBytesPerWord()); > >> } > >> > >> So, the question is why did you use the getAddressSize() but not the > >> getBytesPerWord()? > >> Do they always return the same number? > >> Just would like to understand your reasoning. :) > >> > >> Some nits: > >> > >> test/serviceability/sa/TestInstanceKlassSize.java > >> > >> 138 for (String s:output.asLines()) { > >> 139 if (s.contains (instanceKlassName)) { > >> 140 Asserts.assertTrue( > >> 141 s.contains (jcmdInstanceKlassSize), > >> ... > >> 166 for (String SAInstanceKlassName:SAInstanceKlassNames) { > >> > >> > >> A space is needed after the ':' sign at L138, L166. Space is not > >> needed after the 'contains' at L139, L141. > >> test/serviceability/sa/TestInstanceKlassSizeForInterface.java > >> > >> 138 for (String s:SAOutput.asLines()) { > >> 139 if (s.contains (instanceKlassName)) { > >> 140 Asserts.assertTrue( > >> 141 s.contains (jcmdInstanceKlassSize), > >> > >> A space is needed after the ':' sign at L138. Space is not needed > >> after the 'contains' at L139, L141. Thanks, Serguei On 7/10/16 > 19:07, > >> Jini George wrote: > >> > >> Hi, > >> > >> > >> > >> Gentle Reminder! > >> > >> > >> > >> Thanks, > >> > >> Jini. > >> > >> > >> > >> *From:*Jini George *Sent:* Tuesday, July 05, 2016 9:54 PM *To:* > >> [email protected] > >> <mailto:[email protected]> *Subject:* RFR: > >> JDK-8145627 (sun.jvm.hotspot.oops.InstanceKlass::getSize() > returns > >> the incorrect size and has no test) > >> > >> > >> > >> Hi all, > >> > >> > >> > >> Please review the fix in Serviceability Agent (SA) for: > >> > >> JDK-8145627 > >> <https://bugs.openjdk.java.net/browse/JDK- > 8145627>(sun.jvm.hotspot.oops.InstanceKlass::getSize() > >> returns the incorrect size and has no test) > >> > >> > >> > >> The webrev is at: > >> > >> > <http://cr.openjdk.java.net/%7Esballal/sponsorship/8145627/webrev.00/>h > ttp://cr.openjdk.java.net/~sballal/sponsorship/8145627/webrev.00/ > >> > >> > >> > >> > >> The modifications include the fix and 2 tests to check the > >> InstanceKlass sizes representing some well known classes and > that > >> of an interface. The tests compare the sizes returned by SA to > >> those returned by jcmd. At this point, SA cannot view the > >> InstanceKlasses representing anonymous classes. (This issue will > >> be addressed in JDK-8160228 > >> <https://bugs.openjdk.java.net/browse/JDK-8160228> (SA cannot > view > >> the LambdaMetaFactory generated anonymous instanceKlasses)). > Hence > >> the current fix does not include the size addition for > >> InstanceKlasses representing anonymous classes. > >> > >> > >> > >> Thanks, > >> > >> - Jini Susan George > >> > >> > >> > >
