Re: Low-Overhead Heap Profiling

2017-05-16 Thread JC Beyler 
Dear Robbin,

Thank you for the answers, much appreciated :)
Jc

On Tue, May 16, 2017 at 5:20 AM, Robbin Ehn  wrote:

> Just a few answers,
>
> On 05/15/2017 06:48 PM, JC Beyler wrote:
>
>> Dear all,
>>
>> I've updated the webrev to:
>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/ <
>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/>
>>
>
> I'll look at this later, thanks!
>
>
>> Robbin,
>> I believe I have addressed most of your items with webrev 02:
>>- I added a JTreg test to show how it works:
>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/raw_fi
>> les/new/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c <
>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/raw_f
>> iles/new/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c>
>>   - I've modified the code to use its own data structures both internally
>> and externally, this will make it easier to move out of AsyncGetCallTrace
>> as we move forward, that is still on my TODOs
>>   - I cleaned up the JVMTI API by passing a structure that handles the
>> num_traces and put in a ReleaseTraces as well
>>   - I cleaned up other issues as well.
>>
>> However, I have three questions, which are probably because I'm new in
>> this community:
>>   1) My previous webrevs were based off of JDK9 by mistake. When I took
>> JDK10 via : hg clone http://hg.openjdk.java.net/jdk10/jdk10 <
>> http://hg.openjdk.java.net/jdk10/jdk10> jdk10
>>   - I don't see code compatible with what you were showing (ie your
>> patches don't make sense for that code base; ex: klass is still accessed
>> via klass() for example in collectedHeap.inline.hpp)
>>   - Would you know what is the right hg clone command so we are
>> working on the same code base?
>>
>
> We use jdk10-hs, e.g.
> hg tclone http://hg.openjdk.java.net/jdk10/hs 10-hs
>
> There is sporadic big merges going from jdk9->jdk10->jdk10-hs and
> jdk10-hs->jdk10, so 10 is moving...
>
>
>>   2) You mentioned I was using os::malloc, new, NEW_C_HEAP_ARRAY; I
>> cleaned out the os::malloc but which of the new vs NEW_C_HEAP_ARRAY should
>> I use. It might be that I don't understand when one uses one or the other
>> but I see both used around the code base?
>> - Is it that new is to be used for anything internal and
>> NEW_C_HEAP_ARRAY anything provided to the JVMTI users outside of the JVM?
>>
>
> We overload new operator when you extend correct base class, e.g.
> CHeapObj so use 'new'
> But for arrays you will need the macro NEW_C_HEAP_ARRAY.
>
>
>>   3) Casts: same kind question: which should I use. The code was using a
>> bit of everything, I'll refactor it entirely but I was not clear if I
>> should go to C casts or C++ casts as I see both in the codebase. What is
>> the convention I should use?
>>
>
> Just be consist, use what suites you, C++ casts might be preferable, if we
> are moving towards C++11.
> And use 'right' cast, e.g. going from Thread* to JavaThread* you should
> use C cast or static_cast, not reinterpret_cast I would say.
>
>
>> Final notes on this webrev:
>>- I am still missing:
>>  - Putting a TLAB implementation so that we can compare both webrevs
>>  - Have not tried to circumvent AsyncGetCallTrace
>>  - Putting in the handling of GC'd objects
>>  - Fix a stack walker issue I have seen, I think I know the problem
>> and will test that theory out for the next webrev
>>
>> I will work on integrating those items for the next webrev!
>>
>
> Thanks!
>
>
>> Thanks for your help,
>> Jc
>>
>> Ps:  I tested this on a new repo:
>>
>> hg clone http://hg.openjdk.java.net/jdk10/jdk10 <
>> http://hg.openjdk.java.net/jdk10/jdk10> jdk10
>> ... building it
>> cd test
>> jtreg -nativepath:/build/linux-x86_64-normal-server
>> -release/support/test/hotspot/jtreg/native/lib/ -jdk
>> /linux-x86_64-normal-server-release/images/jdk
>> ../hotspot/test/serviceability/jvmti/HeapMonitor/
>>
>>
> I'll test it out!
>
> /Robbin
>
>
>>
>> On Thu, May 4, 2017 at 11:21 PM, serguei.spit...@oracle.com > serguei.spit...@oracle.com>  serguei.spit...@oracle.com>> wrote:
>>
>> Robbin,
>>
>> Thank you for forwarding!
>> I will review it.
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 5/4/17 02:13, Robbin Ehn wrote:
>>
>> Hi,
>>
>> To me the compiler changes looks what is expected.
>> It would be good if someone from compiler could take a look at
>> that.
>> Added compiler to mail thread.
>>
>> Also adding Serguei, It would be good with his view also.
>>
>> My initial take on it, read through most of the code and took it
>> for a ride.
>>
>> ##
>> - Regarding the compiler changes: I think we need the 'TLAB end'
>> trickery (mentioned by Tony P)
>> instead of a separate check for sampling in fast path for the
>> final version.
>>
>> ##
>> - This patch I had to apply to

Re: Low-Overhead Heap Profiling

2017-05-16 Thread Robbin Ehn 

Just a few answers,

On 05/15/2017 06:48 PM, JC Beyler wrote:

Dear all,

I've updated the webrev to:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/ 



I'll look at this later, thanks!



Robbin,
I believe I have addressed most of your items with webrev 02:
   - I added a JTreg test to show how it works:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/raw_files/new/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c 

  - I've modified the code to use its own data structures both internally and externally, this will make it easier to move out of AsyncGetCallTrace as we move forward, that 
is still on my TODOs

  - I cleaned up the JVMTI API by passing a structure that handles the 
num_traces and put in a ReleaseTraces as well
  - I cleaned up other issues as well.

However, I have three questions, which are probably because I'm new in this 
community:
  1) My previous webrevs were based off of JDK9 by mistake. When I took JDK10 via : hg clone http://hg.openjdk.java.net/jdk10/jdk10  
jdk10
  - I don't see code compatible with what you were showing (ie your patches don't make sense for that code base; ex: klass is still accessed via klass() for example in 
collectedHeap.inline.hpp)

  - Would you know what is the right hg clone command so we are working on 
the same code base?


We use jdk10-hs, e.g.
hg tclone http://hg.openjdk.java.net/jdk10/hs 10-hs

There is sporadic big merges going from jdk9->jdk10->jdk10-hs and 
jdk10-hs->jdk10, so 10 is moving...



  2) You mentioned I was using os::malloc, new, NEW_C_HEAP_ARRAY; I cleaned out the os::malloc but which of the new vs NEW_C_HEAP_ARRAY should I use. It might be that I 
don't understand when one uses one or the other but I see both used around the code base?

- Is it that new is to be used for anything internal and NEW_C_HEAP_ARRAY 
anything provided to the JVMTI users outside of the JVM?


We overload new operator when you extend correct base class, e.g. 
CHeapObj so use 'new'
But for arrays you will need the macro NEW_C_HEAP_ARRAY.



  3) Casts: same kind question: which should I use. The code was using a bit of everything, I'll refactor it entirely but I was not clear if I should go to C casts or C++ 
casts as I see both in the codebase. What is the convention I should use?


Just be consist, use what suites you, C++ casts might be preferable, if we are 
moving towards C++11.
And use 'right' cast, e.g. going from Thread* to JavaThread* you should use C 
cast or static_cast, not reinterpret_cast I would say.



Final notes on this webrev:
   - I am still missing:
 - Putting a TLAB implementation so that we can compare both webrevs
 - Have not tried to circumvent AsyncGetCallTrace
 - Putting in the handling of GC'd objects
 - Fix a stack walker issue I have seen, I think I know the problem and 
will test that theory out for the next webrev

I will work on integrating those items for the next webrev!


Thanks!



Thanks for your help,
Jc

Ps:  I tested this on a new repo:

hg clone http://hg.openjdk.java.net/jdk10/jdk10 
 jdk10
... building it
cd test
jtreg -nativepath:/build/linux-x86_64-normal-server-release/support/test/hotspot/jtreg/native/lib/ -jdk 
/linux-x86_64-normal-server-release/images/jdk ../hotspot/test/serviceability/jvmti/HeapMonitor/




I'll test it out!

/Robbin




On Thu, May 4, 2017 at 11:21 PM, serguei.spit...@oracle.com 
 > wrote:

Robbin,

Thank you for forwarding!
I will review it.

Thanks,
Serguei



On 5/4/17 02:13, Robbin Ehn wrote:

Hi,

To me the compiler changes looks what is expected.
It would be good if someone from compiler could take a look at that.
Added compiler to mail thread.

Also adding Serguei, It would be good with his view also.

My initial take on it, read through most of the code and took it for a 
ride.

##
- Regarding the compiler changes: I think we need the 'TLAB end' 
trickery (mentioned by Tony P)
instead of a separate check for sampling in fast path for the final 
version.

##
- This patch I had to apply to get it compile on JDK 10:

diff -r ac3ded340b35 src/share/vm/gc/shared/collectedHeap.inline.hpp
--- a/src/share/vm/gc/shared/collectedHeap.inline.hppFri Apr 28 
14:31:38 2017 +0200
+++ b/src/share/vm/gc/shared/collectedHeap.inline.hppThu May 04 
10:22:56 2017 +0200
@@ -87,3 +87,3 @@
  // support for object alloc event (no-op most of the time)
-if (klass() != NULL &&

Re: RFR : (JDK9) JDK-8179700 : Exceptions thrown in StartManagementAgent.java

2017-05-16 Thread Daniel Fuchs 

Hi Amit,

Can you point to the changeset (or the reason) which caused
the exception to change?
I'd like to make sure we don't have some regression lurking
here...

best regards,

-- daniel

On 16/05/2017 09:45, Amit Sapre wrote:

Hello,



Please review this trivial fix to StartManagementAgent test case.



Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8179700/webrev.00/

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8179700



Thanks,

Amit**

RFR : (JDK9) JDK-8179700 : Exceptions thrown in StartManagementAgent.java

2017-05-16 Thread Amit Sapre 
Hello,

 

Please review this trivial fix to StartManagementAgent test case.

 

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8179700/webrev.00/

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8179700 

 

Thanks,

Amit