Low-Overhead Heap Profiling

[JC Beyler]

Hi all,

I have a new webrev here:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.15/

With the incremental one here:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.14a_15/

I think I did most items from Thomans and Robbin. I've especially added a
new test:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.14a_15/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.java.patch

This tests the path of object allocation (as opposed to only testing the
array allocation path).

I also updated the JEP:
https://bugs.openjdk.java.net/browse/JDK-8171119

I'll work now with Robbin on the next steps.

Any additional comments/criticisms are as always welcome :)
Jc

On Wed, Nov 8, 2017 at 5:07 AM, Thomas Schatzl <thomas.schatzl at oracle.com>
wrote:

> Hi JC,
>
>   sorry for the long wait.
>
> On Wed, 2017-11-01 at 10:46 -0700, JC Beyler wrote:
> > Dear all,
> >
> > Here is the next webrev:
> > http://cr.openjdk.java.net/~rasbold/8171119/webrev.14a/
> >
> > Incremental since the rebase:
> > http://cr.openjdk.java.net/~rasbold/8171119/webrev.14_14a/
> >
> > (I'm still not too familiar with hg so I had to do a fresh rebase so
> > v14 is once the rebase was done and v14a integrates the changes
> > requested from Thomas).
>
> Yeah, there seem to be something missing in the incremental webrev, but
> thanks for the effort.
>
> > I have also inlined answers to Thomas' email:
> > > A few minor issues:
> >
> > >   - in the declaration of CollectedHeap::sample_allocation, it
> > > would be nice if the fix_sample_rate parameter would be described -
> > > it takes a time to figure out what it's used for. I.e. in case an
> > > allocation goes beyond the sampling watermark, this value which
> > > represents the amount of overallocation is used to adjust the next
> > > sampling watermark to sample at the correct rate.
> > > Something like this - and if what I wrote is incorrect, there is
> > > even more reason to document it.
> > > Or maybe just renaming "fix_sample_rate" to something more
> > > descriptive - but I have no good idea about that.
> > > With lack of units in the type, it would also be nice to have the
> > > unit in the identifier name, as done elsewhere.
> >
>
> Thanks. Could you s/passed/past in that documentation?
>
> > Done for Robbin's issue and changed it to
> > >   - some (or most actually) of the new setters and getters in the
> > > ThreadLocalAllocBuffer class could be private I think. Also, we
> > > typically do not use "simple" getters that just return a member in
> > > the class where they are defined.
> >
> > I removed all that were not used that I could see (not used outside
> > the class) moved the ones that are not simple to private if they
> > could be. I think it's a bit weird because now some of the setting of
> > the tlab internal data is using methods, others are directly setting.
> > Let me know what you think.
>
> That's fine with me. You need to start somewhere I guess.
>
> > >   - ThreadLocalAllocBuffer::pick_next_sample() - I recommend making
> > > the first check an assert - it seems that it is only useful to call
> > > this with heap monitoring enabled, as is done right now.
> >
> > Longer conversation below about this, It cannot be an assert (I could
> > remove the test altogether though).
>
> I looked at the description, and you are right. I missed that. Keep it
> as is. :)
>
> > > - HeapMonitoring::next_random() - the different names for the
> > > constants use different formatting. Preferable (to me) is
> > > UpperCamelCase, but at least make them uniform.
> > >
> >
> > I think done the way you wanted!
>
> In heapMonitoring.hpp:50-53 the constants still have different format?
>
> > >
> > >   - not really convinced that it is a good idea to not somehow
> > > guard
> > > StartHeapSampling() and StopHeapSampling() against being called by
> > > multiple threads.
> > >
> >
> > I added another mutex for the start/stop so that way it will protect
> > from that.
> >
>
> Thanks!
>
> >
> > > Otherwise looks okay from what I can see.
>
> Still okay. I do not need a re-review for the changes suggested in this
> email.
>
> >
> > Awesome, what do you think I still need for this before going to the
> > next step (which is what by the way? :)).
>
> I think:
>
> - look through the JEP if it is still current and fix the descriptions
> if required
>   - add a link to the latest webrev to the JEP as comment
> - if not done already, file CSRs [0] for
>   - the new flag
>   - JVMTI changes (I think, not sure actually)
> - find a sponsor from Oracle to do some internal work (pushing, and
> before that there is iirc still some background work related to JEPs
> that can only be done by Oracle, mostly shepherding :/).
>
> I talked to Robbin about this, and he offered to help you with that.
>
> Thanks,
>   Thomas
>
> [0] https://wiki.openjdk.java.net/display/csr/Main
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171121/1e86e267/attachment-0001.html>