On 7/20/16 00:30, David Holmes wrote:
Just to clarify something ...
On 20/07/2016 5:22 PM, [email protected] 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; [email protected]
*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:*
[email protected]
<mailto:[email protected]> *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
