Review Request (S) 8025841: JVMTI: "vtable stub" dynamic code notification is misplaced

[Oleg Mazurov]

The cost might be minimal but that would be a move in the wrong 
direction in my opinion.
A larger problem is that BufferBlobs being just placeholders for dynamic 
code should not be
reported via JVMTI at all: when they are created they usually contain no 
executable code and
actual objects placed into such blobs are reported separately (that 
wasn't true for vtable/itable
stubs before this fix). That the same address, that of a BufferBlob and 
its first object, could be
reported twice is revealed by a comment to a loop that follows the 
problematic comparison:

src/share/vm/prims/jvmtiCodeBlobEvents.cpp:

  124   // exclude VtableStubs, which are processed separately
  125   if (cb->is_buffer_blob() && strcmp(cb->name(), "vtable chunks") == 0) {
  126     return;
  127   }
  128
  129   // check if this starting address has been seen already - the
  130   // assumption is that stubs are inserted into the list before the
  131   // enclosing BufferBlobs.
  132   address addr = cb->code_begin();
  133   for (int i=0; i<_global_code_blobs->length(); i++) {
  134     JvmtiCodeBlobDesc* scb = _global_code_blobs->at(i);
  135     if (addr == scb->code_begin()) {
  136       return;
  137     }
  138   }

I believe that now that the vtable stub problem is fixed the need for 
that check is gone
and both the strcmp() call and the following loop could be removed 
altogether, thus stopping
further processing for *any* BufferBlob and avoiding a O(n^2) overhead 
they were causing.
The scope of that change is much larger than the original problem 
entailed and would require
not just additional ad hoc testing on the JMTI consumer side but also 
thorough statical analysis
for all BufferBlob uses in the VM code.
In fact, I was going to file a linked JIRA issue on that further 
improvement and if my idea for
it holds true there would be no need for a new BufferBlob subtype.

     -- Oleg

On 2/7/2014 1:06 PM, Dean Long wrote:
> What's the cost for adding a new BufferBlob subtype?  We already have 
> AdapterBlob and MethodHandlesAdapterBlob.
>
> dl
>
> On 2/6/2014 3:52 PM, Oleg Mazurov wrote:
>> My understanding was that a buffer blob was just that - a buffer. 
>> Could potentially contain code fragments of different kinds.
>> Thus, is_buffer_blob() was the closest type available. Agree that a 
>> dependency on its name is not reliable, though testing
>> will reveal if the condition turns false for "vtable chunks" due to a 
>> name change (I had to deal with that particular test, Serguei
>> should be able to identify it). Adding a comment to where the name is 
>> defined (vtableStubs.cpp) that such a dependency exists
>> is a good idea.
>> Thanks,
>>
>>      -- Oleg
>>
>> On Feb 6, 2014, at 3:32 PM, Coleen Phillimore wrote:
>>
>>> Hi, I clicked on this a couple times. It seems okay but isn't there 
>>> a safer way to identify code blobs that are vtable stubs, without 
>>> looking at the name (which can change in while creating it).  A 
>>> comment at least when you create "vtable chunks" would be good.   It 
>>> seems that someone might want to rename it "vtable or itable 
>>> buffers", or something like that.
>>>
>>> thanks,
>>> Coleen
>>>
>>> On 2/6/14 6:17 PM, serguei.spitsyn at oracle.com wrote:
>>>> Runtime team,
>>>>
>>>> This fix was reviewed by Vladimir K. and me.
>>>> Just wanted to make sure if you would like to review it as well.
>>>> If not, then I will push it.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 2/3/14 2:17 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Please, review the fix for:
>>>>>    https://bugs.openjdk.java.net/browse/JDK-8025841
>>>>>
>>>>>
>>>>> Open webrev:
>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/omazurov/8025841-JVMTI-vtbl.1 
>>>>>
>>>>>
>>>>> Summary:
>>>>>
>>>>>    The fix contributed by Oleg Mazurov to improve profiling data 
>>>>> quality.
>>>>>    It moves the "vtable stub" dynamic code notification to the 
>>>>> right place.
>>>>>    I've already reviewed the fix, and it looks good to me.
>>>>>
>>>>>    Bug report description:
>>>>>
>>>>>     "JVMTI_EVENT_DYNAMIC_CODE_GENERATED for "vtable stub" gets 
>>>>> scheduled when
>>>>>      a new chunk of memory for subsequent vtable and itable stubs 
>>>>> is allocated.
>>>>>      That chunk is uninitialized (contains zeros or garbage) 
>>>>> although due to the fact
>>>>>      that the actual event delivery is deferred, at least one 
>>>>> vtable comes out right.
>>>>>
>>>>>      This event should describe an individual vtable/itable stub 
>>>>> (base address and size)
>>>>>      and only after it's been created (memory is actually 
>>>>> populated with code).
>>>>>      Where VM diagnostic messages about vtable/itable stubs are 
>>>>> issued upon
>>>>>      -XX:+PrintAdapterHandlers appears exactly the right place for 
>>>>> JVMTI events as well.
>>>>>
>>>>>      Getting vtables/itables right is important in the context of 
>>>>> performance analysis as
>>>>>      that dynamically generated code may accumulate quite 
>>>>> noticeable CPU time
>>>>>      (especially itabes), sometimes larger than the actual Java 
>>>>> methods called."
>>>>>
>>>>>
>>>>> Testing:
>>>>>    Oleg tested it in the Oracle Studio Performance Analyzer 
>>>>> environment.
>>>>>    nsk.jvmti, nsk.jdi, nsk.jdwp,
>>>>>    In progress: Jtreg com/sun/jdi, java/lang/instrument
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>