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

[Jini Susan George]

Thank you, Serguei. I will be fixing these nits. 

 

Regards,

-Jini 

 

From: Serguei Spitsyn 
Sent: Wednesday, July 20, 2016 12:53 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)

 

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/

 

Would you please take a relook ?

 

Thank you,

Jini.

 

 

From: Serguei Spitsyn 
Sent: Friday, July 15, 2016 3:21 PM
To: Jini George; HYPERLINK "mailto:serviceability-dev at openjdk.java.net"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: HYPERLINK "mailto:serviceability-dev at openjdk.java.net"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:

HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8145627"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/ 

 

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 HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8160228"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/b6ddf221/attachment-0001.html>