<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Alex,<br>
<br>
Nice catch!<br>
<br>
It is about the following fragment:<br>
<pre> 726 for (i = 0; i < thread_stats.number_threads; i++) {
<span class="changed"> 727 if (strcmp(expected_name, thread_stats.threads[i])) {</span>
<span class="changed"> 728 return FALSE;</span>
729 } else {
<span class="changed"> 730 found_thread = TRUE;</span>
731 }
732 }
<span class="new"> 733 return found_thread;</span>
<span class="new"> 734 }</span></pre>
Also, I'd also use 'count' in place of 'number'.<br>
It is to avoid association with thread identification number. <br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<div class="moz-cite-prefix">On 12/11/18 4:42 PM, Alex Menkov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1b8ab7a1-ec08-4397-020e-24463688112b@oracle.com">Hi Jc,
<br>
<br>
The fix looks good.
<br>
The only note is checkThreadSamplesOnlyFrom native function
implementation - the cycle looks confusing.
<br>
As far as I got the function should check that thread_stats
contains only 1 thread and name of the thread is the same as name
of the specified thread.
<br>
And for error analysis it would be great to provide good error
description.
<br>
So I'd make it like
<br>
<br>
if (thread_stats.number_threads != 1) {
<br>
fprintf(stderr, "Wrong thread number: %d (expected 1)\n",
thread_stats.number_threads);
<br>
return FALSE;
<br>
}
<br>
if (strcmp(expected_name, thread_stats.threads[i]) != 0) {
<br>
fprintf(stderr, "Unexpected thread name: '%s' (expected
'%s')\n", thread_stats.threads[i], expected_name);
<br>
return FALSE;
<br>
}
<br>
return TRUE;
<br>
<br>
--alex
<br>
<br>
On 12/11/2018 15:11, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:
<br>
<blockquote type="cite">Hi Jc,
<br>
<br>
Alex will take a look at the test update.
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
On 11/12/18 9:53 AM, JC Beyler wrote:
<br>
<blockquote type="cite">Hi Serguei,
<br>
<br>
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?
<br>
<br>
Thanks!
<br>
Jc
<br>
<br>
On Tue, Nov 6, 2018 at 10:41 PM <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>
<<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>> wrote:
<br>
<br>
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>
<blockquote type="cite"> Hi Serguei,
<br>
<br>
You are right, I should have reverted the
memAllocator.cpp file,
<br>
sorry about that.
<br>
<br>
Here is the new webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.04/">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.04/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.04/"><http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.04/></a>
<br>
<br>
I think we are good by testing standards, like I
<br>
said HeapMonitorThreadTest.java tests multiple threads.
I did
<br>
test an example with a thousand threads and I get the
samples
<br>
from 1000 threads so it seems to work there too.
<br>
<br>
Per thread is tested via the new
<br>
HeapMonitorThreadDisabledTest.java so I think we are
good there too.
<br>
<br>
I would recommend a mach-5 testing just in case for this
one if
<br>
you can, it will be better to have that reinsurance.
<br>
<br>
Thanks for your help,
<br>
Jc
<br>
<br>
On Tue, Nov 6, 2018 at 4:29 PM
<<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>> wrote:
<br>
<br>
Hi Jc,
<br>
<br>
Not sure, I understand a motivation for this change:
<br>
<br>
- if
(JvmtiExport::should_post_sampled_object_alloc()) {
<br>
+ {
<br>
<br>
Also, I'm not sure this is still needed:
<br>
<br>
+#include "prims/jvmtiEventController.inline.hpp"
<br>
+#include "prims/jvmtiThreadState.inline.hpp"
<br>
<br>
I expected you'd just revert all the changes in the
<br>
memAllocator.cpp.
<br>
<br>
Also, it is up to you to make a decision if these
<br>
performance-related fix is needed or not.
<br>
<br>
But it needs to be well tested so that both
global+thread
<br>
event management works correctly.
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
<br>
On 11/6/18 9:42 AM, JC Beyler wrote:
<br>
<blockquote type="cite"> Hi Serguei,
<br>
<br>
Yes exactly it was an optimization. When using a
512k
<br>
sampling rate, I don't see a no real difference
(the
<br>
overhead is anyway low for that sampling rate), I
imagine
<br>
there would be a difference if trying to sample
every
<br>
allocation or with a low sampling interval. But
because you
<br>
are right and it is an optimization of the system
and not a
<br>
functional need, I've reverted it and now the
webrev is
<br>
updated here:
<br>
<br>
Webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.03/">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.03/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.03/"><http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.03/></a>
<br>
Bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8201655">https://bugs.openjdk.java.net/browse/JDK-8201655</a>
<br>
<br>
The incremental webrev is here:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02_03/">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02_03/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02_03/"><http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02_03/></a>
<br>
<br>
Let me know what you think,
<br>
Jc
<br>
<br>
On Mon, Nov 5, 2018 at 6:51 PM
<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>
<br>
<<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>>
wrote:
<br>
<br>
Hi Jc,
<br>
<br>
Okay, I see your point: the change in
memAllocator.cpp
<br>
is for performance.
<br>
Do you have any measurements showing a
performance
<br>
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>
<blockquote type="cite"> Hi Serguei,
<br>
<br>
First off, thanks as always for looking at
this :-) Let
<br>
me inline my answers:
<br>
<br>
I actually "struggled" with this part of the
change. My
<br>
change is correct semantically and if you
care about
<br>
performance for when sampling a given
thread.
<br>
Your change will work semantically but the
performance
<br>
is the same as the global sampling.
<br>
<br>
What I mean by working semantically is that
that the
<br>
tests and the code will work. However, this
means that
<br>
all threads will be doing the sampling work
but when
<br>
the code will post the event here:
<br>
->
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html</a><br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html"><http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html></a><br>
<br>
(which is why your suggestion works, the
change in
<br>
jvmtiExport basically ensures only the
threads
<br>
requested are posting events)
<br>
<br>
The code will check that we actually only
post for
<br>
threads we care about. The code above
ensures that only
<br>
threads that were requested to be sampling
are the ones
<br>
that are sampling internally.
<br>
<br>
Note: I REALLY prefer your suggestion for
two reasons:
<br>
- We do not change the runtime/GC code at
all, it
<br>
remains "simple"
<br>
- The overhead in the general case goes
away and this
<br>
is a NOP for my actual use-case from a
performance
<br>
point of view (sampling every thread)
<br>
<br>
But:
<br>
- Then sampling per thread really is just
telling the
<br>
system don't pollute the callbacks, though
internally
<br>
you are doing all the work anyway.
<br>
<br>
Let me know which you prefer :)
<br>
<br>
<br>
Also, do you see that enabling the
sampling events globally still works?
<br>
<br>
<br>
Yes, otherwise HeapMonitorThreadTest.java
would fail
<br>
since it checks that.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html">http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html</a><br>
<br>
A couple of places where err is
declared as int instead of jvmtiError:
<br>
714 int err;
<br>
742 int err; Should not be silent in a
case of
<br>
JVMTI error: 744 err =
(*jvmti)->GetThreadInfo(jvmti, thread, &info);
<br>
745 if (err != JVMTI_ERROR_NONE) {
<br>
746 return;
<br>
<br>
<br>
<br>
Done and done, I added a fprintf on stderr
saying the
<br>
GetThreadInfo failed and the test is
ignoring the add
<br>
count.
<br>
<br>
Thanks again for looking and let me know
what you think,
<br>
Jc
<br>
<br>
On Mon, Nov 5, 2018 at 2:25 PM
<br>
<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>
<br>
<<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:serguei.spitsyn@oracle.com"><mailto:serguei.spitsyn@oracle.com></a>> wrote:
<br>
<br>
Hi Jc,
<br>
<br>
It looks good in general but I have some
comments
<br>
below.
<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/gc/shared/memAllocator.cpp.udiff.html">http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/gc/shared/memAllocator.cpp.udiff.html</a><br>
<br>
+static bool
thread_enabled_for_one_jvmti_env() {
<br>
+ JavaThread *thread =
JavaThread::current();
<br>
+ JvmtiThreadState *state =
<br>
thread->jvmti_thread_state();
<br>
+ if (state == NULL) {
<br>
+ return false;
<br>
+ }
<br>
+
<br>
+ JvmtiEnvThreadStateIterator it(state);
<br>
+ for (JvmtiEnvThreadState* ets =
it.first(); ets
<br>
!= NULL; ets = it.next(ets)) {
<br>
+ if
<br>
(ets->is_enabled(JVMTI_EVENT_SAMPLED_OBJECT_ALLOC)) {
<br>
+ return true;
<br>
+ }
<br>
+ }
<br>
+
<br>
+ return false;
<br>
+}
<br>
+
<br>
void
MemAllocator::Allocation::notify_allocation_jvmti_sampler()
{
<br>
// support for JVMTI VMObjectAlloc
event (no-op if not enabled)
<br>
JvmtiExport::vm_object_alloc_event_collector(obj());
<br>
if
(!JvmtiExport::should_post_sampled_object_alloc()) {
<br>
// Sampling disabled
<br>
return;
<br>
}
<br>
+ // Sampling is
enabled for at least one thread,
<br>
is it this one?
<br>
+ if
(!thread_enabled_for_one_jvmti_env()) {
<br>
+ return;
<br>
+ }
<br>
+ I don't think you need this change as
this
<br>
condition already does it: if
(!JvmtiExport::should_post_sampled_object_alloc()) {
<br>
// Sampling disabled
<br>
return;
<br>
}
<br>
<br>
Please, look at the following line in
the jvmtiEventController.cpp:
<br>
JvmtiExport::set_should_post_sampled_object_alloc((any_env_thread_enabled
& SAMPLED_OBJECT_ALLOC_BIT) != 0);
<br>
<br>
I hope, testing will prove my
suggestion is correct.
<br>
Also, do you see that enabling the
sampling events globally still works?
<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html">http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c.frames.html</a><br>
<br>
A couple of places where err is
declared as int instead of jvmtiError:
<br>
714 int err;
<br>
742 int err; Should not be silent in a
case of
<br>
JVMTI error: 744 err =
(*jvmti)->GetThreadInfo(jvmti, thread, &info);
<br>
745 if (err != JVMTI_ERROR_NONE) {
<br>
746 return;
<br>
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
<br>
On 10/26/18 10:48, JC Beyler wrote:
<br>
<blockquote type="cite"> Hi all,
<br>
<br>
When working on the heap sampling, I
had promised
<br>
to do the per thread event so here it
is!
<br>
<br>
Could I get a review for this:
<br>
Webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/">http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/"><http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/></a>
<br>
Bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8201655">https://bugs.openjdk.java.net/browse/JDK-8201655</a>
<br>
<br>
I was thinking of adding GC-dev for
the
<br>
memAllocator change once I get
favorable reviews
<br>
for the rest of the change.
<br>
<br>
I've done a bit of performance testing
and on the
<br>
Dacapo benchmark I see no change in
performance
<br>
when turned off (logical, any code
change is
<br>
behind a flag check already in place)
and when
<br>
turned on it is comparable to the
current performance.
<br>
<br>
(More information is: I see a very
slight
<br>
degradation if we are doing 512k
sampling but no
<br>
degradation at 2MB).
<br>
<br>
Thanks,
<br>
Jc
<br>
</blockquote>
<br>
<br>
<br>
-- <br>
Thanks,
<br>
Jc
<br>
</blockquote>
<br>
<br>
<br>
-- <br>
Thanks,
<br>
Jc
<br>
</blockquote>
<br>
<br>
<br>
-- <br>
Thanks,
<br>
Jc
<br>
</blockquote>
<br>
<br>
<br>
-- <br>
<br>
Thanks,
<br>
Jc
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>