PING: RFR: JDK-8145627 (sun.jvm.hotspot.oops.InstanceKlass::getSize() returns the incorrect size and has no test)

[Jini Susan George]

Thank you, David.

Regards,
Jini.

> -----Original Message-----
> From: David Holmes
> Sent: Wednesday, July 20, 2016 1:01 PM
> To: Serguei Spitsyn; Jini George; serviceability-dev at openjdk.java.net
> 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, serguei.spitsyn at oracle.com 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; serviceability-dev at openjdk.java.net
> >> *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:*
> >>     serviceability-dev at openjdk.java.net
> >>     <mailto:serviceability-dev at openjdk.java.net> *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
> >>
> >>
> >>
> >