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

Fri Oct 2 13:44:28 PDT 2009

Hi Dan,

I'm 90% of the way done incorporating the changes that you've suggested. There are two changes left that I have yet to make and wanted to ask clarification about.

1) You mention,

>- the "next" field is bothering me due to it making the data stream
>  difficult to copy and relocate, but regular JVM/TI does pointers
>  in returned data structures also.

Can you clarify what you mean by this? 

2) You mention,

> >line 1756
 > >record->methods[record->numstackframes] = sd->method()->jmethod_id();

>This code does not check for a NULL method() return, but
>the code that calls this function does. I'm guessing that
>means that it is possible for sd->method() to return NULL
>here...

>Dan

One solution to this would be to insert a null check that marks the whole JvmtiCompiledMethodLoadInlineRecord as invalid if sd->method() returns null. (In the header we could hash-define a constant value that indicates an invalid record). A JVMTI agent would then have to check that the record is valid before proceeding to use it. Would this be an acceptable solution?

Regards,

Vasanth

-----Original Message-----
From: Daniel.Daugherty at Sun.COM [mailto:Daniel.Daugherty at Sun.COM] 
Sent: Thursday, September 24, 2009 4:43 PM
To: Venkatachalam, Vasanth
Cc: hotspot-dev at openjdk.java.net; Pollan, Ben; Thomas.Rodriguez at Sun.COM
Subject: Re: Request for reviews (M): 6580131: Exposing Method Inlining InformationTo JVMTI Agents

Vasanth,

Sorry it has taken me so long to get to this review.

- is this targeted at OpenJDK6 or OpenJDK7?
- original notes says needs 64-bit testing; still true?
- will this need CCC approval? I hope not since this is a VM specific
  data stream that we're exporting. However ...
- need to check with Kelly/Tim to see the right way to get the
  header file exported in the JDK.
- the samples need some cleanup, but that's a side issue; they could
  be used as the basis for some new tests.

src/share/vm/code/jvmticmlr.h

 > struct JvmtiCompiledMethodLoadRecordHeader_s

- the "_s" suffix is not same style as JVM/TI; JVM/TI style is
  typedef struct _foo foo.
- the "next" field is bothering me due to it making the data stream
  difficult to copy and relocate, but regular JVM/TI does pointers
  in returned data structures also.
- version info should be explicit rather than bundled in "type" field
- need to add version info constants, perhaps a major and minor number

 > struct JvmtiCompiledMethodLoadInlineRecord_s

- The header field comment makes things a little more clear about the
  layout of this data. I'm thinking that the "void *" points to a blob
  of memory that we cast to:

  - JvmtiCompiledMethodLoadRecordHeader * which tells us the "type" of
    this record and tells us where the next record is or NULL.
  - JvmtiCompiledMethodLoadInlineRecord * which allows us to decode
    this first record
  - the "methods" and "bcis" fields possibly point to data that
    immediately follows the JvmtiCompiledMethodLoadInlineRecord;
    could be that all of the methods arrays and all of the bcis
    arrays are together in one place rather than interleaved

  Update: now that I've gone back and read the original e-mail, it is
  much more clear, but should some of this info be in the header file?

 > struct JvmtiCompiledMethodLoadDummyRecord_s

- header comment mentions JvmtiCompiledMethodLoadRecord, but
  that does not exist

src/share/vm/prims/jvmtiExport.cpp

 > line 1737
 > int numstackframes = 0;

This should be a "jint" just to match types.

 > line 1743
 > "Assertion violation in add_inline_records:pc_address must be non-null"

Drop the "Assertion violation in add_inline_records:" part; we
know it's an assertion failure, the system will tell us where
it is and the function name is wrong.

 > line 1753
 > record->numstackframes = 0;

You've already counted the frames by looping on 1748-1749. Why not set
that value here rather than count again? If you're thinking that the
ScopeDesc list size can change, then you need to check for an overflow
in the loop on line 1755.

 > line 1754
 > "Assertion violation in jvmtiExport::add_inline_records: 
numstackframes must be nonzero."

Drop the "Assertion violation in jvmtiExport::add_inline_records:" part

 > line 1756
 > record->methods[record->numstackframes] = sd->method()->jmethod_id();

This code does not check for a NULL method() return, but
the code that calls this function does. I'm guessing that
means that it is possible for sd->method() to return NULL
here...

Dan

Venkatachalam, Vasanth wrote:
>
> Hi Tom,
>
> Thank you again for your detailed and insightful review. We have 
> revised our implementation based on your suggestions and submitted new 
> diffs for bugid 100085 with the changes described below. We have 
> attached these diffs and new sample JVMTI agents to the bug report at:
>
>  
>
> https://bugs.openjdk.java.net/show_bug.cgi?id=100085
>
>  
>
>  
>
> 1. Overall Structure
>
>
> We have added one new header file to the Hotspot JVM, jvmticmlr.h. 
>  This header defines a struct JvmtiCompiledMethodLoadRecordHeader that 
> represents arbitrary information passed through the void pointer. The 
> struct has a type field to indicate the type of information being 
> passed, and a next pointer. The type field can also be used to capture 
> versioning information.
>
>  
>
> We also defined another struct, JvmtiCompiledMethodLoadInlineRecord, 
> which contains the previous struct as the first member but in addition 
> has extra fields for storing inlining information. One can similarly 
> define additional structs to pass other kinds of information. As an 
> example, we've defined a struct JvmtiCompiledMethodLoadDummyRecord, 
> which contains a string "I am a dummy record." When the sample JVMTI 
> agents that we have provided encounter a record of this type, they 
> print the string contained in the dummy record. 
>
>  
>
> This new header file jvmticmlr.h lives in the Hotspot 
> src/share/vm/code directory. However, we envision that the JDK 
> installer would copy it to the JDK include/ directory to make this 
> file available to external JVMTI agents, in the same way that it makes 
> jni.h available. 
>
>
> When a CompiledMethodLoad event is generated for an nmethod, the void 
> pointer will contain a list of JvmtiCompiledMethodLoadInlineRecords. 
> Each record has a pc address which actually represents a range of pc 
> addresses between that JvmtiCompiledMethodLoadInlineRecord and the pc 
> address of the next one in the list.
>
>  
>
> When the sample JVMTI agents we have provided detect a 
> CompiledMethodLoad event, they first cast the void pointer,of the 
> CompiledMethodLoad callback  to a JvmtiCompiledMethodLoadRecordHeader 
> *  list, and iterate over each of the records in the list, checking 
> its type and then casting the record to the appropriate subtype 
> (e.g., JvmtiCompiledMethodLoadInlineRecords) before deciding how to 
> process it.
>
>
> 2. Code Changes
>
>  
>
> The main code changes are to the jvmtiExport.cpp file, located in 
> src/share/vm/prims. We have defined a new global method, 
> create_inline_records which returns a list of records containing 
> inline information for a given nmethod. The post_compiled_method_load 
> method of JvmtiExport calls this method to retrieve the 
> inlining information and pass it rhrough the void pointer.
>
>  
>
> In addition to these changes, we have made the following miscellaneous 
> changes that were requested:
>
>  
>
> -We changed the date of the copyright notices in the new header files 
> to 2009.
> -We have changed all function names to use lowercase with underscores.
> -We replaced null checks of impossible conditions with assertions as 
> recommended.
> -We removed the unused type and offset fields from the data structure 
> that was previously called InlineInfoRecord and is now called 
> JVMTICompiledMethodLoadInlineRecord. We renamed the jmethodID[] array 
> to methods and bytecodeindexes to bcis.
> -We removed the PCDescriptorList class since it was redundant.
>
> -We modified our code to use resource allocation rather than C heap 
> allocations. Please see the create_inline_records method in 
> jvmtiExport.cpp.
> -We modified jvmtiExport.cpp to set the _compile_info void pointer 
> inside the JvmtiCompiledMethodLoadEventMark constructor, rather than 
> setting this pointer in a separate method call.
>
>  
>
> 3. Sample Agents
>
> The sample agents now use the jvmticmlr.h header file. For ease of 
> testing, we have copied this header file from the JVM
>
> src/share/vm/code directory into the c and cpp directories containing 
> the agents. However, as noted before, the JDK installer would ideally 
> copy this header file to the JDK /include directory, so that it can be 
> made available in the same way that jni.h is available.
>  
>
> Again, we thank you for your detailed feedback. Please let us know if 
> you have additional comments or questions
>
>  
>
> Regards,
>
>  
>
> Vasanth
>
>  
>
>  
>
> *From:* Venkatachalam, Vasanth
> *Sent:* Tuesday, August 04, 2009 10:47 AM
> *To:* 'hotspot-dev at openjdk.java.net'
> *Cc:* 'Thomas.Rodriguez at sun.com'; Joshi, Shrinivas; Pollan, Ben
> *Subject:* RE: Request for reviews (M): 6580131: Exposing Method 
> Inlining InformationTo JVMTI Agents
>
>  
>
> Hi Tom,
>
>  
>
> Thank you for your detailed and insightful feedback on my JVMTI method 
> inlining submission. Sorry for the delay in responding to your 
> comments. We are scoping out the changes that you have recommended. 
> Some of these changes could take longer, but we hope to be able to 
> respond in a few days with some changes or at least a timeline on when 
> we can get them in.
>
>  
>
> Regards,
>
>  
>
> Vasanth
>
>  
>
>  
>
> --
>
> Vasanth Venkatachalam
>
> AMD Java Labs
>
> (512)602-6177
>
>  
>