RFR 8171119: Low-Overhead Heap Profiling

[Erik Österlund]

Hi JC,

I have looked through your changes.

In threadLocalAllocBuffer.cpp:

  349 HeapWord* ThreadLocalAllocBuffer::allocate_sampled_object(size_t 
size) {
  350   Thread* thread = myThread();
  351   thread->tlab().set_back_allocation_end();
  352   HeapWord* result = thread->tlab().allocate(size);
  353
  354   if (result) {
  355     thread->heap_sampler().check_for_sampling(result, size * 
HeapWordSize, _bytes_since_last_sample_point);
  356     thread->tlab().set_sample_end();
  357   }
  358
  359   return result;
  360 }

...it looks like you are fetching myThread() from the TLAB, but use this 
fetched thread only to fetch the TLAB. But That TLAB is in fact "this" 
in this context. I would prefer not calling x() instead of 
thread->tlab()->x().

I don't need another webrev for this. The rest looks good to me. Thank 
you for doing this, and thank you for breaking out the per-thread logic 
into its own class.

Thanks,
/Erik

On 2018-05-15 18:57, JC Beyler wrote:
> Hi Thomas,
>
> Thanks for the review! I've done your two fixes for the next webrev. 
> I'll perhaps wait for the next review to combine webrev updates? Your 
> two change requests were minimal it might make sense to wait before 
> re-generating a new webrev.
>
> Anybody else have any comments/suggestions?
>
> Here are the latest links:
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.20/ 
> <http://cr.openjdk.java.net/%7Ejcbeyler/8171119/heap_event.20/>
> JEP-331 Issue: https://bugs.openjdk.java.net/browse/JDK-8171119
> CSR: https://bugs.openjdk.java.net/browse/JDK-8194905
> Test Plan: https://bugs.openjdk.java.net/browse/JDK-8202566
>
> Thanks again all!
> Jc
>
> On Tue, May 15, 2018 at 12:23 AM Thomas Schatzl 
> <thomas.schatzl at oracle.com <mailto:thomas.schatzl at oracle.com>> wrote:
>
>     Hi,
>
>     On Mon, 2018-05-14 at 13:02 -0700, JC Beyler wrote:
>     > Hi Robbin and all,
>     >
>     > Thank you for your continuous help!
>     >
>     > Done then! Here is the new webrev:
>     > http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.20/
>     <http://cr.openjdk.java.net/%7Ejcbeyler/8171119/heap_event.20/>
>     >
>     > and the incremental is:
>     > http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.19_20/
>     <http://cr.openjdk.java.net/%7Ejcbeyler/8171119/heap_event.19_20/>
>     >
>     > Thanks again all!
>     > Jc
>     >
>
>       looks good to me.
>
>     Two minor issues that you might want to fix before pushing:
>
>     - collectedHeap.hpp: The declaration of
>     CollectedHeap::allocate_memory() should be private.
>
>     - collectedHeap.inline.hpp: in CollectedHeap::allocate_memory() there
>     is this completely duplicated code which you might factor out into
>     another (private) method:
>
>     if (init_memory) {
>       obj = ...
>     } else {
>       obj = ...
>     }
>     post_setup(klass, ...);
>
>     Thanks,
>       Thomas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180517/498173a6/attachment-0001.html>