Hi Denghui, I think there should be one event type per pool, or there should be a text field specifying which pool it is.
If we believe there will be many pools in the future, it may make sense for a field, otherwise I think an event type could be used. The benefit of one event type per pool is that there can be a description per pool and it also allows easy filtering and configuration. If there would be four pools, they could all derive from an abstract event class and only add @Label and @Description per event subclass. If there would be forty pools, it would be too much noise and a field would be better. Erik > On 31 Mar 2020, at 11:06, Denghui Dong <denghui....@alibaba-inc.com> wrote: > > PING. > > @Erik Could you review it? > > Denghui Dong > > > > ------------------------------------------------------------------ > From:董登辉(卓昂) <denghui....@alibaba-inc.com> > Send Time:2020年3月19日(星期四) 16:35 > To:Alan Bateman <alan.bate...@oracle.com>; Erik Gahlin > <erik.gah...@oracle.com> > Cc:hotspot-jfr-dev <hotspot-jfr-...@openjdk.java.net>; core-libs-dev > <core-libs-dev@openjdk.java.net> > Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics > > Hi, > > On Mar 18, 2020, at 11:46 PM, Alan Bateman <alan.bate...@oracle.com > <mailto:alan.bate...@oracle.com>> wrote: > > > > On 13/03/2020 14:54, Denghui Dong wrote: > Good suggestion, moved. > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ > <http://cr.openjdk.java.net/~ddong/8238665/webrev.03/> > This looks much better. > > What would you think about renaming the JFR event to > "DirectBufferStatistics"? The concern I have with the proposed naming is that > it will be really awkward to extend it to support mapped buffers. > > It’s ok for me. > > > The implementation changes look okay, hopefully Erik will skim over them. One > small suggestion for for ManagementFactoryHelper is that you can > stream().collect(Collectors.toList()) to create the value for bufferPools. > You could even change it to volatile so that getBufferMXBeans isn't a > synchronized method. > > > Make sense, updated. > > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/ > <http://cr.openjdk.java.net/~ddong/8238665/webrev.04/> > > @Erik, could you help review it? > > > -Alan. > > Denghui Dong
