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

[David Holmes]

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/>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; 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/>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 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
>>
>>
>>
>