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

[Dean Long]

OK.  Your further improvement idea sounds promising.

dl

On 2/7/2014 3:55 PM, Oleg Mazurov wrote:
> 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
>>>>>>
>>
>