Hi Denghui, The copyright headers should have the classpath exception, similar to other files in the same directory.
If somebody in core-libs could review this, especially the modification of src/java.base/share/classes/module-info.java, I can the push this. Thanks Erik > On 5 Mar 2020, at 03:44, Denghui Dong <denghui....@alibaba-inc.com> wrote: > > Hi Erik, > Updated. > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/ > <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/> > > Thanks > Denghui Dong > ------------------------------------------------------------------ > From:Erik Gahlin <erik.gah...@oracle.com> > Send Time:2020年3月5日(星期四) 06:33 > To:董登辉(卓昂) <denghui....@alibaba-inc.com> > Cc:hotspot-jfr-dev <hotspot-jfr-...@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@openjdk.java.net <mailto:core-libs-dev@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.gah...@oracle.com > <mailto:erik.gah...@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....@alibaba-inc.com > <mailto:denghui....@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/ > <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.gah...@oracle.com <mailto:erik.gah...@oracle.com>> > Send Time:2020年3月3日(星期二) 04:08 > To:董登辉(卓昂) <denghui....@alibaba-inc.com <mailto:denghui....@alibaba-inc.com>> > Cc:hotspot-jfr-dev <hotspot-jfr-...@openjdk.java.net > <mailto:hotspot-jfr-...@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 > <https://bugs.openjdk.java.net/browse/JDK-8238665> > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/ > <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/> > > ------------------------------------------------------------------ > From:董登辉(卓昂) <denghui....@alibaba-inc.com> > <mailto:denghui....@alibaba-inc.com> > Send Time:2020年2月11日(星期二) 16:13 > To:Erik Gahlin <erik.gah...@oracle.com> <mailto:erik.gah...@oracle.com> > Cc:hotspot-jfr-dev <hotspot-jfr-...@openjdk.java.net> > <mailto:hotspot-jfr-...@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/ > <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/> > > On Feb 11, 2020, at 1:52 AM, Erik Gahlin <erik.gah...@oracle.com > <mailto:erik.gah...@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 > <https://bugs.openjdk.java.net/browse/JDK-8238665> > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.00/ > <http://cr.openjdk.java.net/~ddong/8238665/webrev.00/> > Test: test/jdk/jdk/jfr/event/runtime/TestDirectMemoryStatisticsEvent.java > > Thanks > Denghui Dong > > >
