Request for reviews (M): 6580131: Exposing Method Inlining InformationTo JVMTI Agents

[Venkatachalam, Vasanth]

Hi Dan,

Thanks again for your feedback. We've submitted new diffs for bugid 100085 with the changes you requested below. We've attached these diffs and new sample JVMTI agents to the bug report at:

https://bugs.openjdk.java.net/show_bug.cgi?id=100085

A few notes:

-The jvmticmlr.h header file now uses the commenting convention used for struct definitions in the $ JAVA_HOME/include files. We fixed typos and made wording changes as requested.

-In jvmtiExport.cpp, we removed the count_pc_descriptors method and inlined its code into the create_inline_record method.

-In jvmtiExport.cpp, there was a line that read:

assert(!(sd->method() == NULL),

You recommended changing this to:

assert(sd->method() != NULL)

This causes a build error on Windows suggesting that the "!=" operator doesn't apply to the type methodHandle.
I searched the Hotspot sources for similar use patterns. In nmethod.cpp, line 2103, the is_null() function is being used for null-checking the result of the sd->method() call.
Based on this, I changed the above line to read:

assert(!sd->method().is_null())

Aside from this issue, we've made all the other changes you have requested. Please advise us as to the next steps.

Regards,

Vasanth


From: Daniel.Daugherty at Sun.COM [mailto:Daniel.Daugherty at Sun.COM]
Sent: Thursday, November 12, 2009 5:38 PM
To: Venkatachalam, Vasanth
Cc: Thomas.Rodriguez at Sun.COM; hotspot-dev at openjdk.java.net; Pollan, Ben; Joshi, Shrinivas; Frost, Gary
Subject: Re: Request for reviews (M): 6580131: Exposing Method Inlining InformationTo JVMTI Agents

Comments on the latest version:

src/share/vm/code/jvmticmlr.h

One thing I didn't notice before: Sun comment style is to have a space
after the "//" and before the comment starts, e.g., "// this is a comment",
but it appears that the $JAVA_HOME/include header files never use the "//"
comment form so those should probably switch to "/* ... */" form...


typo: "used used to derive" -> "used to derive"

> Initialized to 0 in ...

Should be "Initialized to current version value in ..."

And the version constants should be defined in the header file
instead of in the jvmtiExport.cpp file. See the *_VERSION_*
values in jni.h for an example. I don't think you really need
the masking defines and stuff that jvmti.h has...

typo: "at the pc address pc" -> "at the pc address"

> An dynamic array of records that indicate what methods...

Shouldn't this be "array of records that indicate what methods..."?
The count should match the previous "numpcs" value and not be
dynamic in this context.

> JVMTI_DUMMYRECORD

This should be JVMTI_DUMMYINFORECORD to match the previous constant's
naming style.

When we get the jvmticmlr.h file contents nailed down I
get the ball rolling on the CCC request so we can get
approval to get this into JDK7/HSX-17.


src/share/vm/prims/jvmtiExport.cpp

One thing I didn't notice before: Sun comment style is to have a space
after the "//" and before the comment starts, e.g., "// this is a comment"

Also, you have a mix of "/*...*/ and "//" comments in the new stuff. Why?

Nit: initial indent in count_pc_descriptors() is one space and should
     be two spaces.

Nit: inconsistent space around "=" in init of new version numbers.

The "assert(numstackframes != 0, ..." should be right after the value
is calculated and not after the memory allocs.

> assert(!(sd->method() == NULL), ...

Would be more clear as "assert(sd->method() != NULL, ..."

Curious: why have "count_pc_descriptors()" as a separate method and
do the numstackframes counting loop in-line?

Dan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20091117/b39f2c07/attachment-0001.html