Re: RFR: 8238665: Add JFR event for direct memory statistics

Thu Mar 5 02:44:26 UTC 2020

Hi Erik,
Updated.
Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/

Thanks
Denghui Dong
------------------------------------------------------------------
From:Erik Gahlin <erik.gahlin at oracle.com>
Send Time:2020年3月5日(星期四) 06:33
To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>
Cc:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi Denghui, 

Looking through you patch again.  

Could you add copyright headers, and change “5000 ms” to “5 s” in the .jfc files. When you reply with the updated patch, could you please include core-libs-dev at openjdk.java.net in the reply since you are modifying exports in java.base.

Thanks
Erik

On 4 Mar 2020, at 20:35, Erik Gahlin <erik.gahlin at oracle.com> wrote:
Hi Denghui, 

I have tested your fix and it seems to work. TestDefaultConfiguration takes into account the event is periodic, so it doesn't complain as I expected. 

Looks good. I can sponsor this.

Thanks
Erik

On 3 Mar 2020, at 07:14, Denghui Dong <denghui.ddh at alibaba-inc.com> wrote:
Hi Erik,
I agree with you that the default interval should be changed to 5s, the webrev (http://cr.openjdk.java.net/~ddong/8238665/webrev.01/) has been updated.
No need to modify TestDefaultConfigurations.java, it is passed.

Testing
(Linux release/slow debug build): jdk/jfr, all passed except TestNativeLibrariesEvent and TestNetworkUtilizationEvent(unstable). And these two test failures were not caused by this patch.
And there were some test errors in slow debug build, I have checked the error test list and confirm they are also error in slow debug build without this patch, such as TestRecordedFrame.

There are two new files in this patch, should I add copyright header to them?

Thanks
Denghui Dong
------------------------------------------------------------------
From:Erik Gahlin <erik.gahlin at oracle.com>
Send Time:2020年3月3日(星期二) 04:08
To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>
Cc:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi Denghui,

 Thanks for the explanation. 
I think the default interval could be increased to once every 5 seconds.
Have you run the all the tests in test/jdk/jdk/jfr? I would expect TestDefaultConfigurations to have failed unless modified.
Thanks
 Erik
On 2020-02-26 14:40, 董登辉(卓昂) wrote:
PING: Could you review it?
Bug: https://bugs.openjdk.java.net/browse/JDK-8238665
Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/

------------------------------------------------------------------
From:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>
Send Time:2020年2月11日(星期二) 16:13
To:Erik Gahlin <erik.gahlin at oracle.com>
Cc:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi Erik,

Thanks for the review. See answers embedded.

New webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/

On Feb 11, 2020, at 1:52 AM, Erik Gahlin <erik.gahlin at oracle.com> wrote: 
Hi Denghiu,

 Can you explain when this event can be useful, i.e. when troubleshooting x etc.? This makes it easier to determine if the event should be on by default, if it is worth the increased startup cost that the instrumentation creates and what a reasonable period for the event should be.

Many Java middleware(e.g. Netty) and applications use direct memory for achieving higher performance,  if java process use too much direct memory, some exceptions will occur, such as oom-killer. Hence, it’s necessary to monitor it,  and we use JMX to monitor it in our production environment currently.

src/jdk.jfr/share/classes/jdk/jfr/events/DirectMemoryStatisticsEvent.java:

 @Label("TotalCapacity") -> @Label("Total Capacity")
 @Label("MemoryUsed") -> @Label("Memory Used”)

 Updated

What is meant by 'limit'? Could we have a better field name and/or description that explains what this fields contains, because it is not obvious from that name. Fields that contains bytes, should have the DataAmount; annotation.

 What is the value if -XX:MaxDirectMemorySize has not been set?

limit -> maxCapacity

 if -XX:MaxDirectMemorySize has not been set, the value of Runtime.getRuntime().maxMemory() will be used. 
Can you make the event class final.

 Updated

@StackTrace(false) should not be needed.

 Removed

test/jdk/jdk/jfr/event/runtime/TestDirectMemoryStatisticsEvent.java:

 Use try-with-resources on the Recording object.

 Updated

Thanks
Denghui Dong

Thanks
 Erik

On 2020-02-07 06:24, Denghui Dong wrote:
 Hi,

 Could I have a review of a change that adds JFR event for direct memory statistics

 Bug: https://bugs.openjdk.java.net/browse/JDK-8238665
 Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.00/
 Test: test/jdk/jdk/jfr/event/runtime/TestDirectMemoryStatisticsEvent.java

 Thanks
 Denghui Dong