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

[JC Beyler]

Hi Serguei,

First off, thanks as always for looking at this :-) Let me inline my
answers:

I actually "struggled" with this part of the change. My change is correct
semantically and if you care about performance for when sampling a given
thread.
Your change will work semantically but the performance is the same as the
global sampling.

What I mean by working semantically is that that the tests and the code
will work. However, this means that all threads will be doing the sampling
work but when the code will post the event here:
   ->
http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html

(which is why your suggestion works, the change in jvmtiExport basically
ensures only the threads requested are posting events)

The code will check that we actually only post for threads we care about.
The code above ensures that only threads that were requested to be sampling
are the ones that are sampling internally.

Note: I REALLY prefer your suggestion for two reasons:
  - We do not change the runtime/GC code at all, it remains "simple"
  - The overhead in the general case goes away and this is a NOP for my
actual use-case from a performance point of view (sampling every thread)

But:
  - Then sampling per thread really is just telling the system don't
pollute the callbacks, though internally you are doing all the work anyway.

Let me know which you prefer :)



>  Also, do you see that enabling the sampling events globally still works?
>
>
Yes, otherwise HeapMonitorThreadTest.java would fail since it checks that.


> http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html <http://cr.openjdk.java.net/~jcbeyler/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;
>
>
>
Done and done, I added a fprintf on stderr saying the GetThreadInfo failed
and the test is ignoring the add count.

Thanks again for looking and let me know what you think,
Jc

On Mon, Nov 5, 2018 at 2:25 PM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:

> 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:
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8201655
>
> 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
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181105/e27d277f/attachment.html>