Re: 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.scha...@oracle.com <mailto:thomas.scha...@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