RFR 8171119: Low-Overhead Heap Profiling

[JC Beyler]

Hi Erik,

Thanks for the review. I changed this method to:
 HeapWord* ThreadLocalAllocBuffer::allocate_sampled_object(size_t size) {
-  Thread* thread = myThread();
-  thread->tlab().set_back_allocation_end();
-  HeapWord* result = thread->tlab().allocate(size);
+  set_back_allocation_end();
+  HeapWord* result = allocate(size);

   if (result) {
-    thread->heap_sampler().check_for_sampling(result, size * HeapWordSize,
_bytes_since_last_sample_point);
-    thread->tlab().set_sample_end();
+    myThread()->heap_sampler().check_for_sampling(result, size *
HeapWordSize, _bytes_since_last_sample_point);
+    set_sample_end();
   }

I still have a myThread() call due to calling the heap_sampler but moved it
below in the if (result) and then did the calls to x() directly instead of
thread->tlab()->x() as requested.

Thanks for the review and your help! I'll post a new webrev in a bit after
it builds and passes my HeapMonitor tests,
Jc


On Thu, May 17, 2018 at 2:45 AM Erik Österlund <erik.osterlund at oracle.com>
wrote:

> 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/
> 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>
> 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/
>> >
>> > and the incremental is:
>> > http://cr.openjdk.java.net/~jcbeyler/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/400af4c3/attachment.html>