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

[Ioi Lam]

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;
...
};

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.

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 
>>>
>>
>