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

[Ioi Lam]

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

> thanks,
>
> Chris
>
> https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#DynamicCodeGenerated 
>