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

[serguei.spitsyn at oracle.com]

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.

   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/~sballal/sponsorship/8145627/webrev.01/ 
> <http://cr.openjdk.java.net/%7Esballal/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/~sballal/sponsorship/8145627/webrev.00/
>     <http://cr.openjdk.java.net/%7Esballal/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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160720/49424faa/attachment-0001.html>