Low-Overhead Heap Profiling

[Thomas Schatzl]

Hi Jc,

  sorry for taking a bit long to respond.... ;)

On Mon, 2017-10-23 at 08:27 -0700, JC Beyler wrote:
> Dear all,
> 
> Small update this week with this new webrev:
>   - http://cr.openjdk.java.net/~rasbold/8171119/webrev.13/
>   - Incremental is here: http://cr.openjdk.java.net/~rasbold/8171119/
> webrev.12_13/
> 
> I patched the code changes showed by Robbin last week and I
> refactored collectedHeap.cpp:
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.12_13/src/hotspot/
> share/gc/shared/collectedHeap.cpp.patch
> 
> The original code became a bit too complex in my opinion with the
> handle_heap_sampling handling too many things. So I subdivided the
> logic into two smaller methods and moved out a bit of the logic to
> make it more clear. Hopefully it is :)
> 
> Let me know if you have any questions/comments :)
> Jc

A few minor issues:

  - weak reference handling has been factored out in JDK-8189359, now
you only need to add the additions required for this change to one
place. :) Please update the webrev :)

  - the one issue Robin noticed.

  - 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.

  - 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.

  - ThreadLocalAllocBuffer::set_sample_end(): please use
pointer_delta() for pointer subtractions.

  - 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.

  - ThreadLocalAllocBuffer::pick_next_sample() - please use
"PTR_FORMAT" (or INTPTR_FORMAT - they are the same) as format string
for printing pointer values as is customary within Hotspot. %p output
is OS dependent. I.e. I heard that e.g. on Ubuntu it prints "null"
instead of 0x0...0 .... which is kind of annoying.

  - personal preference: do not allocate
HeapMonitoring::AlwaysTrueClosure globally, but only locally when it's
used. Setting it up seems to be very cheap.

  - HeapMonitoring::next_random() - the different names for the
constants use different formatting. Preferable (to me) is
UpperCamelCase, but at least make them uniform.

  - in HeapMonitoring::next_random(), you might want to use
right_n_bits() to create your mask.

  - not really convinced that it is a good idea to not somehow guard
StartHeapSampling() and StopHeapSampling() against being called by
multiple threads.

Otherwise looks okay from what I can see. 

Thanks,
  Thomas