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

[serguei.spitsyn at oracle.com]

On 7/20/16 00:30, David Holmes wrote:
> 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.

Right.
I thought, it is clear. :)
Thank you for clarifying.

Thanks,
Serguei


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