Low-Overhead Heap Profiling
jcbeyler at google.com
Tue Nov 21 21:50:15 UTC 2017
I have a new webrev here:
With the incremental one here:
I think I did most items from Thomans and Robbin. I've especially added a
This tests the path of object allocation (as opposed to only testing the
array allocation path).
I also updated the JEP:
I'll work now with Robbin on the next steps.
Any additional comments/criticisms are as always welcome :)
On Wed, Nov 8, 2017 at 5:07 AM, Thomas Schatzl <thomas.schatzl at oracle.com>
> 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.
> > > Otherwise looks okay from what I can see.
> Still okay. I do not need a re-review for the changes suggested in this
> > 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  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.
>  https://wiki.openjdk.java.net/display/csr/Main
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev