Low-Overhead Heap Profiling

[Thomas Schatzl]

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