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

[serguei.spitsyn at oracle.com]

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 *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/20160715/6660517e/attachment-0001.html>