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

[Tom Rodriguez]

So I have a bunch of issues with the code itself but before even  
getting into that I have a larger question.  This is essentially a new  
public API to HotSpot so we need a standard header file that can be  
used to get the definition of the data we're passing.  Where is that  
going to live and where is this going to be documented?  I also think  
that we need to define some sort of header for the compile_info  
argument so that it can be properly identified and versioned assuming  
that we might extend or improve it in the future.

As far as the code itself:

Function names should be all lower case with underscores.

Don't use CHeapObj::operator new for C heap allocations.  Use  
NEW_C_HEAP_ARRAY.  Actually I don't see why it can't simply use  
resource allocation instead.  There's an appropriate ResourceMark at  
the use site.

PCDescriptorList should inherit from StackObj since it is designed to  
be used with stack discipline.

We don't use the ifndef/define idiom for header files.

The date in the copyright notice for the new files is wrong.

the jint type field of InlineInfoRecord is unused.

Why does it contain both a pc and an offset?  I think the pc is  
sufficient given the other information in the event.  The jmethodID[]  
should be named methods instead of frames and bytecodeindexes should  
simply be bcis.

The comment on PCDescriptorList says that it's using arena allocation  
which isn't true.  It also needs at least a couple sentences of  
comments describing what it does and how.

The "if(pclist != NULL && pclist->tail != NULL)" checks in  
addActivationRecord and intializeActivationRecords should really be  
asserts.  Failing that test indicates a misuse of the API.  Same with  
the pclist == NULL test in addPCDescriptor.

I think the passed out void * should have some facility for  
identifying what's being passed and versioning that data.  Passing out  
a raw pointer seems like a bad idea.

The _compile_info pointer should be passed through the constructor  
instead of being set separately.
get_pc_descriptors should be part of PCDescriptorList instead of  
nmethod since it's implementable using only public API.
I think the checks for null and numstackframes != 0 should be asserts  
since those should be impossible values.

tom

On Jul 28, 2009, at 4:14 PM, Vladimir Kozlov wrote:

> http://cr.openjdk.java.net/~kvn/6580131/webrev.00
>
> Fixed 6580131: CompiledMethodLoad events don't produce the expected  
> extra notifications to describe inlining
>
> and
>
> https://bugs.openjdk.java.net/show_bug.cgi?id=100085 Exposing Method  
> Inlining Information To JVMTI Agents
>
> Problem: (from 6580131 Description)
> The spec for JVMTI says or implies that CompiledMethodLoad
> should generate extra events to describe the inlining in
> a piece of compiled code.  Currently we don't do this which
> really limits the amount of information available for methods
> with heavy inlining.  We should probably be producing the extra
> events, assuming that existing tools don't barf when they
> get them.
>
> Solution: (from 100085 Description)
> Whenever Hotspot JVM compiles a method, it creates
> a list of records that specify what method invocations are
> on the compile-time stack at each program address. Hotspot JVM
> was extended to make this information visible to external
> tools through the JVMTI layer, without altering the JVMTI
> specification. The main change was to pass this stack
> information through the CompiledMethodLoad JVMTI callback.
> As a result, external tools can use this information
> to produce better source-to-address mapping.
>
> Contributed-by: Vasanth.Venkatachalam at amd.com
>
> Reviewed by:
>
> Other testing:
> JPRT
>