<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Jc,<br>
<br>
Alex will take a look at the test update.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<div class="moz-cite-prefix">On 11/12/18 9:53 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBy9iXYiFqxAn=c07Pa8=8rg2=LndGJqwJVerkVHbxiz8Q@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">Hi Serguei,<br>
<div><br>
</div>
<div>Thanks for the update and thanks for testing mach5. Serguei
sent me that the testing passed mach5 testing, could I get
another review to be able to push it?</div>
<div><br>
</div>
<div>Thanks!</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Tue, Nov 6, 2018 at 10:41 PM <a
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> <<a
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_-1674171603098455828moz-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">
<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/"
target="_blank" 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:serguei.spitsyn@oracle.com"
target="_blank" moz-do-not-send="true">serguei.spitsyn@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_-1674171603098455828m_4822938364167385442removed">- if (JvmtiExport::should_post_sampled_object_alloc()) {</span>
<span class="m_-1674171603098455828m_4822938364167385442new">+ {</span></pre>
Also, I'm not sure this is still needed:<br>
<pre><span class="m_-1674171603098455828m_4822938364167385442new">+#include "prims/jvmtiEventController.inline.hpp"</span>
<span class="m_-1674171603098455828m_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_-1674171603098455828m_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:serguei.spitsyn@oracle.com"
target="_blank" moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
<<a
href="mailto:serguei.spitsyn@oracle.com"
target="_blank" moz-do-not-send="true">serguei.spitsyn@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-im">
<div><br>
</div>
<div> <br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-im">
<div> <br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224moz-cite-prefix">
<pre><a class="m_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224new">
</span> 714 int err;
<span class="m_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224changed"> 746 return;</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554gmail-m_5462246381949391541gmail-m_-3385713941130615224changed"></span><span class="m_-1674171603098455828m_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:serguei.spitsyn@oracle.com"
target="_blank"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
<<a
href="mailto:serguei.spitsyn@oracle.com"
target="_blank"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div
class="m_-1674171603098455828m_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_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+static bool thread_enabled_for_one_jvmti_env() {</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ JavaThread *thread = JavaThread::current();</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ JvmtiThreadState *state = thread->jvmti_thread_state();</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ if (state == NULL) {</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return false;</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ JvmtiEnvThreadStateIterator it(state);</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = it.next(ets)) {</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ if (ets->is_enabled(JVMTI_EVENT_SAMPLED_OBJECT_ALLOC)) {</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return true;</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return false;</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+}</span>
<span class="m_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ // Sampling is enabled for at least one thread, is it this one?</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ if (!thread_enabled_for_one_jvmti_env()) {</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ return;</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">+ }</span>
<span class="m_-1674171603098455828m_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_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224new">
</span> 714 int err;
<span class="m_-1674171603098455828m_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_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224changed"> 746 return;</span>
<span class="m_-1674171603098455828m_4822938364167385442m_7443063805677570592m_-7679167297582474554m_-3385713941130615224changed"></span><span class="m_-1674171603098455828m_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_-1674171603098455828m_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_-1674171603098455828m_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_-1674171603098455828m_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="m_-1674171603098455828gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br clear="all">
<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>