[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8201655: Add thread-enabled support for the Heap Sampling
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2018-12-12 20:17:52
Message-ID: 3f49676e-acc7-3d03-ebb0-14e8e2fbe708 () oracle ! com
[Download RAW message or body]
LGTM++
Thanks,
Serguei
On 12/12/18 12:15, Alex Menkov wrote:
> Looks good to me.
>
> --alex
>
> On 12/12/2018 11:25, JC Beyler wrote:
> > Thanks both for the review, I fixed both issues and here is the new
> > webrev, let me know what you think:
> >
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.06/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8201655
> >
> > Thanks!
> > Jc
> >
> > On Tue, Dec 11, 2018 at 5:01 PM <serguei.spitsyn@oracle.com
> > <mailto:serguei.spitsyn@oracle.com>> wrote:
> >
> > Hi Alex,
> >
> > Nice catch!
> >
> > It is about the following fragment:
> >
> > 726 for (i = 0; i < thread_stats.number_threads; i++) {
> > 727 if (strcmp(expected_name, thread_stats.threads[i])) {
> > 728 return FALSE;
> > 729 } else {
> > 730 found_thread = TRUE;
> > 731 }
> > 732 }
> > 733 return found_thread;
> > 734 }
> >
> > Also, I'd also use 'count' in place of 'number'.
> > It is to avoid association with thread identification number.
> >
> > Thanks,
> > Serguei
> >
> >
> > On 12/11/18 4:42 PM, Alex Menkov wrote:
> > > Hi Jc,
> > >
> > > The fix looks good.
> > > The only note is checkThreadSamplesOnlyFrom native function
> > > implementation - the cycle looks confusing.
> > > 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.
> > > And for error analysis it would be great to provide good error
> > > description.
> > > So I'd make it like
> > >
> > > if (thread_stats.number_threads != 1) {
> > > fprintf(stderr, "Wrong thread number: %d (expected 1)\n",
> > > thread_stats.number_threads);
> > > return FALSE;
> > > }
> > > if (strcmp(expected_name, thread_stats.threads[i]) != 0) {
> > > fprintf(stderr, "Unexpected thread name: '%s' (expected
> > > '%s')\n", thread_stats.threads[i], expected_name);
> > > return FALSE;
> > > }
> > > return TRUE;
> > >
> > > --alex
> > >
> > > On 12/11/2018 15:11, serguei.spitsyn@oracle.com
> > > <mailto:serguei.spitsyn@oracle.com> wrote:
> > > > Hi Jc,
> > > >
> > > > Alex will take a look at the test update.
> > > >
> > > > Thanks,
> > > > Serguei
> > > >
> > > > On 11/12/18 9:53 AM, JC Beyler wrote:
> > > > > Hi Serguei,
> > > > >
> > > > > 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?
> > > > >
> > > > > Thanks!
> > > > > Jc
> > > > >
> > > > > On Tue, Nov 6, 2018 at 10:41 PM serguei.spitsyn@oracle.com
> > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > <mailto:serguei.spitsyn@oracle.com> <serguei.spitsyn@oracle.com
> > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > <mailto:serguei.spitsyn@oracle.com>> wrote:
> > > > >
> > > > > Hi Jc,
> > > > >
> > > > > Thank you for the update!
> > > > > It looks good.
> > > > > It is great that testing on your side is Okay.
> > > > >
> > > > > I'll submit a mach5 job soon (today or tomorrow).
> > > > >
> > > > > Thanks,
> > > > > Serguei
> > > > >
> > > > >
> > > > > On 11/6/18 20:03, JC Beyler wrote:
> > > > > > Hi Serguei,
> > > > > >
> > > > > > You are right, I should have reverted the memAllocator.cpp
> > > > > > file,
> > > > > > sorry about that.
> > > > > >
> > > > > > Here is the new webrev:
> > > > > > http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.04/
> > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.04/>
> > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.04/>
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > Per thread is tested via the new
> > > > > > HeapMonitorThreadDisabledTest.java so I think we are good
> > > > > > there too.
> > > > > >
> > > > > > I would recommend a mach-5 testing just in case for this
> > > > > > one if
> > > > > > you can, it will be better to have that reinsurance.
> > > > > >
> > > > > > Thanks for your help,
> > > > > > Jc
> > > > > >
> > > > > > On Tue, Nov 6, 2018 at 4:29 PM <serguei.spitsyn@oracle.com
> > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > <mailto:serguei.spitsyn@oracle.com>> wrote:
> > > > > >
> > > > > > Hi Jc,
> > > > > >
> > > > > > Not sure, I understand a motivation for this change:
> > > > > >
> > > > > > - if (JvmtiExport::should_post_sampled_object_alloc()) {
> > > > > > + {
> > > > > >
> > > > > > Also, I'm not sure this is still needed:
> > > > > >
> > > > > > +#include "prims/jvmtiEventController.inline.hpp"
> > > > > > +#include "prims/jvmtiThreadState.inline.hpp"
> > > > > >
> > > > > > I expected you'd just revert all the changes in the
> > > > > > memAllocator.cpp.
> > > > > >
> > > > > > Also, it is up to you to make a decision if these
> > > > > > performance-related fix is needed or not.
> > > > > >
> > > > > > But it needs to be well tested so that both
> > > > > > global+thread
> > > > > > event management works correctly.
> > > > > >
> > > > > > Thanks,
> > > > > > Serguei
> > > > > >
> > > > > >
> > > > > > On 11/6/18 9:42 AM, JC Beyler wrote:
> > > > > > > Hi Serguei,
> > > > > > >
> > > > > > > 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:
> > > > > > >
> > > > > > > Webrev:
> > > > > > > http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.03/
> > > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.03/>
> > > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.03/>
> > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8201655
> > > > > > >
> > > > > > > The incremental webrev is here:
> > > > > > > http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02_03/
> > > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02_03/>
> > > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02_03/>
> > > > > > >
> > > > > > > Let me know what you think,
> > > > > > > Jc
> > > > > > >
> > > > > > > On Mon, Nov 5, 2018 at 6:51 PM
> > > > > > > serguei.spitsyn@oracle.com <mailto:serguei.spitsyn@oracle.com>
> > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > <serguei.spitsyn@oracle.com
> > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > <mailto:serguei.spitsyn@oracle.com>> wrote:
> > > > > > >
> > > > > > > Hi Jc,
> > > > > > >
> > > > > > > Okay, I see your point: the change in
> > > > > > > memAllocator.cpp
> > > > > > > is for performance.
> > > > > > > Do you have any measurements showing a performance
> > > > > > > difference?
> > > > > > > Also, do you need me to submit a mach5 test run?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Serguei
> > > > > > >
> > > > > > >
> > > > > > > On 11/5/18 15:14, JC Beyler wrote:
> > > > > > > > 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
> > > > > > > > <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>
> > > > > > > >
> > > > > > > > (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
> > > > > > > >
> > > > > > > > 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@oracle.com <mailto:serguei.spitsyn@oracle.com>
> > > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > > <serguei.spitsyn@oracle.com
> > > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > > <mailto:serguei.spitsyn@oracle.com>
> > > > > > > > <mailto:serguei.spitsyn@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/
> > > > > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/>
> > > > > > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > > Jc
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > > Jc
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Jc
> > > >
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic