<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
Thank you for the update!<br>
It looks good.<br>
It is great that testing on your side is Okay.<br>
<br>
I'll submit a mach5 job soon (today or tomorrow).<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/6/18 20:03, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi Serguei,
<div><br>
</div>
<div>You are right, I should have reverted the
memAllocator.cpp file, sorry about that.</div>
<div><br>
</div>
<div>Here is the new webrev:</div>
<div><a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.04/</a><br>
</div>
<div><br>
</div>
<div>I think we are good by testing standards, like I
said HeapMonitorThreadTest.java tests multiple threads. I
did test an example with a thousand threads and I get the
samples from 1000 threads so it seems to work there too.</div>
<div><br>
</div>
<div>Per thread is tested via the new
HeapMonitorThreadDisabledTest.java so I think we are good
there too.</div>
<div><br>
</div>
<div>I would recommend a mach-5 testing just in case for
this one if you can, it will be better to have that
reinsurance.</div>
<div><br>
</div>
<div>Thanks for your help,</div>
<div>Jc</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Tue, Nov 6, 2018 at 4:29 PM <<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div> Hi Jc,<br>
<br>
Not sure, I understand a motivation for this change:<br>
<pre><span class="m_4822938364167385442removed">- if (JvmtiExport::should_post_sampled_object_alloc()) {</span>
<span class="m_4822938364167385442new">+ {</span></pre>
Also, I'm not sure this is still needed:<br>
<pre><span class="m_4822938364167385442new">+#include "prims/jvmtiEventController.inline.hpp"</span>
<span class="m_4822938364167385442new">+#include "prims/jvmtiThreadState.inline.hpp"</span></pre>
I expected you'd just revert all the changes in the
memAllocator.cpp.<br>
<br>
Also, it is up to you to make a decision if these
performance-related fix is needed or not.<br>
<br>
But it needs to be well tested so that both global+thread
event management works correctly.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<div class="m_4822938364167385442moz-cite-prefix">On 11/6/18
9:42 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi Serguei,<br>
<div><br>
</div>
<div>Yes exactly it was an optimization. When
using a 512k sampling rate, I don't see a no
real difference (the overhead is anyway low for
that sampling rate), I imagine there would be a
difference if trying to sample every allocation
or with a low sampling interval. But because you
are right and it is an optimization of the
system and not a functional need, I've reverted
it and now the webrev is updated here:</div>
<div><br>
</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.03/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.03/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8201655"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201655</a></div>
<div><br>
</div>
<div>The incremental webrev is here: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02_03/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02_03/</a></div>
<div><br>
</div>
<div>Let me know what you think,</div>
<div>Jc<br>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Nov 5, 2018 at 6:51 PM <a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>
<<a href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554moz-cite-prefix">Hi
Jc,<br>
<br>
Okay, I see your point: the change in
memAllocator.cpp is for performance.<br>
Do you have any measurements showing a performance
difference?<br>
Also, do you need me to submit a mach5 test run?<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/5/18 15:14, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi Serguei,<br>
<div><br>
</div>
<div>First off, thanks as always for looking at
this :-) Let me inline my answers:</div>
<br>
<div class="gmail_quote">
<div>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. </div>
<div>Your change will work semantically but
the performance is the same as the global
sampling. </div>
<div><br>
</div>
<div>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:</div> <div> -> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html</a></div>
<div><br>
</div>
<div>(which is why your suggestion works, the
change in jvmtiExport basically ensures only
the threads requested are posting events)</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Note: I REALLY prefer your suggestion for
two reasons:</div>
<div> - We do not change the runtime/GC code
at all, it remains "simple"</div>
<div> - 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)</div>
<div><br>
</div>
<div>But:</div>
<div> - Then sampling per thread really is
just telling the system don't pollute the
callbacks, though internally you are doing
all the work anyway.</div>
<div><br>
</div>
<div>Let me know which you prefer :)<br>
</div>
<span
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-im">
<div><br>
</div>
<div> <br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224moz-cite-prefix">
<pre> Also, do you see that enabling the sampling events globally still works?</pre>
</div>
</div>
</blockquote>
<div><br>
</div>
</span>
<div>Yes, otherwise HeapMonitorThreadTest.java
would fail since it checks that.</div>
<span
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-im">
<div> <br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224moz-cite-prefix">
<pre><a class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html" target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html</a>
A couple of places where err is declared as int instead of jvmtiError:
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224new">
</span> 714 int err;
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224changed"> 742 int err;
Should not be silent in a case of JVMTI error: </span> 744 err = (*jvmti)->GetThreadInfo(jvmti, thread, &info);
745 if (err != JVMTI_ERROR_NONE) {
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224changed"> 746 return;</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224changed"></span><span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224new"></span></pre>
<br>
</div>
</div>
</blockquote>
<div><br>
</div>
</span>
<div>Done and done, I added a fprintf on
stderr saying the GetThreadInfo failed and
the test is ignoring the add count.</div>
<div><br>
</div>
<div>Thanks again for looking and let me know
what you think,</div>
<div>Jc</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Nov 5, 2018 at 2:25 PM <a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>
<<a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224moz-cite-prefix">Hi
Jc,<br>
<br>
It looks good in general but I have some
comments below.<br>
<br>
<br>
<a
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/gc/shared/memAllocator.cpp.udiff.html"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/gc/shared/memAllocator.cpp.udiff.html</a><br> <br> <pre><span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+static bool thread_enabled_for_one_jvmti_env() {</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ JavaThread *thread = JavaThread::current();</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ JvmtiThreadState *state = thread->jvmti_thread_state();</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ if (state == NULL) {</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return false;</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ JvmtiEnvThreadStateIterator it(state);</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = it.next(ets)) {</span> <span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ if (ets->is_enabled(JVMTI_EVENT_SAMPLED_OBJECT_ALLOC)) {</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return true;</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return false;</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+}</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+</span>
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;
}
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ // Sampling is enabled for at least one thread, is it this one?</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ if (!thread_enabled_for_one_jvmti_env()) {</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return;</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+
I don't think you need this change as this condition already does it:
</span> 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?
<a class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html" target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html</a>
A couple of places where err is declared as int instead of jvmtiError:
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">
</span> 714 int err;
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224changed"> 742 int err;
Should not be silent in a case of JVMTI error: </span> 744 err = (*jvmti)->GetThreadInfo(jvmti, thread, &info);
745 if (err != JVMTI_ERROR_NONE) {
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224changed"> 746 return;</span>
<span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224changed"></span><span class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new"></span></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 10/26/18 10:48, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,</div>
<div dir="ltr"><br>
</div>
<div>When working on the heap
sampling, I had promised to do the
per thread event so here it is! </div>
<div><br>
</div>
<div dir="ltr">Could I get a review
for this:<br>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/</a><br>
</div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8201655"
target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201655</a></div>
<div><br>
</div>
<div>I was thinking of adding GC-dev
for the memAllocator change once I
get favorable reviews for the rest
of the change.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>(More information is: I see a
very slight degradation if we are
doing 512k sampling but no
degradation at 2MB). </div>
<div><br>
</div>
<div dir="ltr"
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224gmail_signature">
<div dir="ltr">Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_4822938364167385442m_7443063805677570592gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>