Re: RFR (M) 8201655: Add thread-enabled support for the Heap Sampling

Mon, 05 Nov 2018 14:28:00 -0800

Hi Jc,

It looks good in general but I have some comments below.


http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/gc/shared/memAllocator.cpp.udiff.html

+static bool thread_enabled_for_one_jvmti_env() {
+  JavaThread *thread  = JavaThread::current();
+  JvmtiThreadState *state = thread->jvmti_thread_state();
+  if (state == NULL) {
+    return false;
+  }
+
+  JvmtiEnvThreadStateIterator it(state);
+  for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = it.next(ets)) {
+    if (ets->is_enabled(JVMTI_EVENT_SAMPLED_OBJECT_ALLOC)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 void MemAllocator::Allocation::notify_allocation_jvmti_sampler() {
   // support for JVMTI VMObjectAlloc event (no-op if not enabled)
   JvmtiExport::vm_object_alloc_event_collector(obj());
 
   if (!JvmtiExport::should_post_sampled_object_alloc()) {
     // Sampling disabled
     return;
   }
 
+  // Sampling is enabled for at least one thread, is it this one?
+  if (!thread_enabled_for_one_jvmti_env()) {
+    return;
+  }
+

 I don't think you need this change as this condition already does it:
   if (!JvmtiExport::should_post_sampled_object_alloc()) {
     // Sampling disabled
     return;
   }

 Please, look at the following line in the jvmtiEventController.cpp:
    JvmtiExport::set_should_post_sampled_object_alloc((any_env_thread_enabled & SAMPLED_OBJECT_ALLOC_BIT) != 0);

 I hope, testing will prove my suggestion is correct.
 Also, do you see that enabling the sampling events globally still works?


http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html

 A couple of places where err is declared as int instead of jvmtiError:
  
 714   int err;
 742   int err;

 Should not be silent in a case of JVMTI error:

 744   err = (*jvmti)->GetThreadInfo(jvmti, thread, &info);
 745   if (err != JVMTI_ERROR_NONE) {
 746     return;

Thanks,
Serguei


On 10/26/18 10:48, JC Beyler wrote:
Hi all,

When working on the heap sampling, I had promised to do the per thread event so here it is! 

Could I get a review for this:

I was thinking of adding GC-dev for the memAllocator change once I get favorable reviews for the rest of the change.

I've done a bit of performance testing and on the Dacapo benchmark I see no change in performance when turned off (logical, any code change is behind a flag check already in place) and when turned on it is comparable to the current performance.

(More information is: I see a very slight degradation if we are doing 512k sampling but no degradation at 2MB). 

Thanks,
Jc

Reply via email to