RFR(10)(XS): JDK-8171365: nsk/jvmti/scenarios/events/EM04/em04t001: many errors for missed events

[Chris Plummer]

On 6/5/17 6:57 PM, Ioi Lam wrote:
>
>
> On 6/5/17 1:30 PM, Chris Plummer wrote:
>> On 6/5/17 8:56 AM, Ioi Lam wrote:
>>> Hi Chris,
>>>
>>>
>>> On 6/2/17 11:54 AM, Chris Plummer wrote:
>>>> Hello,
>>>>
>>>> [I'd like a compiler team member to comment on this, in addition to 
>>>> runtime or svc]
>>>>
>>>> Please review the following:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8171365
>>>> http://cr.openjdk.java.net/~cjplummer/8171365/webrev.00/
>>>>
>>>> The CR is closed, so I'll describe the issue here:
>>>>
>>>> The test is making sure that all 
>>>> |JVMTI_EVENT_DYNAMIC_CODE_GENERATED| events that occur during the 
>>>> agent's OnLoad phase also occur when later GenerateEvents() is 
>>>> called. GenerateEvents() is generating some of the events, but most 
>>>> are not sent. The problem is CodeCache::blobs_do() is only 
>>>> iterating over NMethod code heaps, not all of the code heaps, so 
>>>> many code blobs are missed. I changed it to iterate over all the 
>>>> code heaps, and now all the 
>>>> |JVMTI_EVENT_DYNAMIC_CODE_GENERATED|events are sent.
>>>>
>>>> Note there is another version of CodeCache::blobs_do() that takes a 
>>>> closure object instead of a function pointer. It is used by GC and 
>>>> I assume is working properly by only iterating over NMethod code 
>>>> heaps, so I did not change it. The version that takes a function 
>>>> pointer is only used by JVMTI for implementing GenerateEvents(), so 
>>>> this change should not impact any other part of the VM. However, I 
>>>> do wonder if these blobs_do() methods should be renamed to avoid 
>>>> confusion since they don't (and haven't in the past) iterated over 
>>>> the same set of code blobs.
>>>>
>>> Yes, I think these two function would indeed be confusing. The 
>>> second variant also does a liveness check which is missing from the 
>>> first one:
>>>
>>>  621 void CodeCache::blobs_do(void f(CodeBlob* nm)) {
>>>  622   assert_locked_or_safepoint(CodeCache_lock);
>>>  623   FOR_ALL_HEAPS(heap) {
>>>  624     FOR_ALL_BLOBS(cb, *heap) {
>>>  625       f(cb);
>>>  626     }
>>>  627   }
>>>  628 }
>>>
>>>  664 void CodeCache::blobs_do(CodeBlobClosure* f) {
>>>  665   assert_locked_or_safepoint(CodeCache_lock);
>>>  666   FOR_ALL_NMETHOD_HEAPS(heap) {
>>>  667     FOR_ALL_BLOBS(cb, *heap) {
>>>  668       if (cb->is_alive()) {
>>>  669         f->do_code_blob(cb);
>>>  670 #ifdef ASSERT
>>>  671         if (cb->is_nmethod())
>>>  672         ((nmethod*)cb)->verify_scavenge_root_oops();
>>>  673 #endif //ASSERT
>>>  674       }
>>>  675     }
>>>  676   }
>>>  677 }
>>>
>>> The two function's APIs are equivalent, since CodeBlobClosure has a 
>>> single function, so I think it's better to stick with one API, i.e. 
>>> replace the function pointer with CodeBlobClosure
>>>
>>> class CodeBlobClosure : public Closure {
>>>  public:
>>>   // Called for each code blob.
>>>   virtual void do_code_blob(CodeBlob* cb) = 0;
>>> };
>>>
>>> For consistency, maybe we should change the first version to
>>>
>>> CodeCache::all_blobs_do(CodeBlobClosure* f)
>>>
>>> and the second to
>>>
>>> CodeCache::live_nmethod_blobs_do(CodeBlobClosure* f)
>>>
>>> Thanks
>>> - Ioi
>> Hi Ioi,
>>
>> Thanks for the review.
>>
>> I don't see a good reason why a closure should be used here. There's 
>> no state belonging to the iteration that needs to be accessed by the 
>> callback. It's just a simple functional callback, so it seems 
>> converting this to use a closure is overkill. I'd have to say the 
>> same is true of the closure version of do_blobs also. Why use a 
>> closure for that?
>>
> Some of the closures actually have instance fields. E.g.,
>
> class G1CodeBlobClosure : public CodeBlobClosure {
>   class HeapRegionGatheringOopClosure : public OopClosure {
>     ...
>   };
>
>   HeapRegionGatheringOopClosure _oc;
> ...
> };
Ok, I missed that. So at least some of the gc usage requires a closure. 
Not sure that's enough to make JVMTI switch over to it.
>
> I think in general using a closure is better than a function pointer. 
> It makes evolution of the code easier when in the future you need to 
> handle more complicated situations that would require states. Not sure 
> what the HotSpot convention is, but my preference would be to use 
> closures unless there's a strict performance requirement for using a 
> function pointer.
I guess I have the opposite thinking here. Unnecessary abstraction 
drives me batty, and I like to keep it simple unless there's clear 
future need for something more flexible. We have one call site in JVMTI 
for this method, and a new user ever did require a closure, JVMTI could 
be changed at that time.

I'll wait for the compiler team to chime in. I've already asked them to 
have a look (this issue not withstanding) because it is their code.

thanks,

Chris
>
>
> Thanks
> - Ioi
>
>
>> BTW, there also:
>>
>> void CodeCache::nmethods_do(void f(nmethod* nm)) {
>>   assert_locked_or_safepoint(CodeCache_lock);
>>   NMethodIterator iter;
>>   while(iter.next()) {
>>     f(iter.method());
>>   }
>> }
>>
>> So another functional version of a blob iterator. It's not real clear 
>> to me all the ways this differs from the closure version of 
>> blobs_do(), other than it allows you to provide an nmethod as a 
>> starting point for the iteration.
>>
>> Chris
>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#DynamicCodeGenerated 
>>>>
>>>
>>
>