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

[Chris Plummer]

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?

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