[RFR]: Per thread IO statistics in JFR
David Holmes
david.holmes at oracle.com
Thu Jan 17 07:36:56 UTC 2019
On 17/01/2019 5:23 pm, Thomas Stüfe wrote:
>
>
> On Thu, Jan 17, 2019 at 1:23 AM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> On 17/01/2019 12:59 am, Alan Bateman wrote:
> >
> > Thanks for forwarding as this is a topic that will require a lot of
> > discussion and agreement from several areas due to potential
> impact on
> > long term maintenance and the potential for some of these native
> methods
> > to go away.
> >
> > Gunter - can you summarize the approaches that you have explored? In
> > particular, it would be useful to see a write-up of approaches that
> > don't change the native methods to call JVM_* functions at every I/O
> > call.
>
> Indeed! Under the adage "you shouldn't pay for what you don't use" this
> seems an unacceptable penalty on I/O operations if you don't want to
> gather the event info - and possibly excessive even if you do! The
> dynamic instrumentation approach was adopted because static
> instrumentation like this was not considered appropriate.
>
>
> You noticed that all Gunther does is to increment stat counters in the
> VM, he does not generate JFR events at all?
>
> Do you object against keeping these counters (which basically boils down
> to Thread::current->stat_structure->counter++)? Or do you even object
> against making upcalls into the jvm? Note that, if deemed necessary, we
> could omit updating the counters unless JFR or our extended thread dumps
> are activated (which are the consumers of the counters).
It is the "call JVM_* functions at every I/O call" that I am concerned
about!
> In any case, I would have assumed the costs for upcall + counter update
> to be insignificant compared to the IO calls. We should of course
> measure that.
It's going to depend on the actual I/O operation of course.
>
> If you generally object upcalls into the libjvm for
> statistical/monitoring reasons, this would make matters on a number of
> fronts more complicated. For instance, it was discussed extending NMT
> coverage to the JDK - which is already in part reality at
> Unsafe.AllocateMemory - and this would have to be done with upcalls too.
And it would need to be examined in detail too.
Cheers,
David
-----
> Kind Regards, Thomas
>
>
> David
> -----
>
> > It would also be useful to understand how it compares to dynamic
> > instrumentation today (once there are suitable java methods to
> > instrument). Maybe someone working on JFR can comment on that too.
> >
> > -Alan
> >
> > On 16/01/2019 14:25, Thomas Stüfe wrote:
> >> Hi Gunter,
> >>
> >> (I remove svc but add corelibs and hs-runtime for the jdk/hs parts,
> >> respectively. Hope this is not too spammy).
> >>
> >> This looks all very good.
> >>
> >> I am not a JFR reviewer (is there such a thing?) but only a JDK
> reviewer,
> >> so I am not sure if I am allowed to review the JFR parts.
> >>
> >> That said, here it goes:
> >>
> >> General remarks:
> >>
> >> A) we may reduce the number of sent events vastly by only
> sending updates
> >> for threads which actually did IO. I know that would require
> expanding
> >> the
> >> ThreadStatisticalInfo to keep JFR-specific counters. But I think
> the
> >> memory
> >> overhead of the increased size of ThreadStatisticalInfo would be
> more
> >> than
> >> offset by the space saved for noop events. What to you think?
> >>
> >> (Side note, it would be nice to have generic support for this in
> the JFR
> >> layer. If the JFR layer could remember the last values for an
> event and
> >> omit sending it if nothing changed.)
> >>
> >> B) You could save a bit of work by folding multiple values into one
> >> event.
> >> Other events seem to do that too, eg. MetaspaceSummary. E.g:
> instead of
> >> having ThreadNetworkWriteStatistics and
> ThreadNetworkReadStatistics, you
> >> could have ThreadNetworkStatistics with "read" and "write"
> fields, and
> >> thus
> >> combine the polling functions for those two. Maybe even combine
> all four
> >> values into a single "ThreadIOStatistics" event.
> >>
> >> Review:
> >>
> >> Hook functions:
> >>
> >> - This is bike shedding, but I would like something like
> "JVM_On<xxx>()"
> >> more, sounding more "hookish", since the functions do not "xxx" but
> >> inform
> >> the hotspot of "xxx" having occurred
> >>
> >> - Also, I wonder whether it would make sense to make the len
> input 64bit
> >> (jlong instead of jint) in case we want to use them to
> instrument 64bit
> >> reads too
> >>
> >> - If you wanted to minimize the number of new JVM_.. hook
> functions, you
> >> could combine network and file io hooks into one:
> >>
> >> JVM_OnIORead(.. bool is_socket)
> >> JVM_OnIOWrite(.. bool is_socket)
> >>
> >> or even JVM_OnIO(.., bool is_read, bool is_socket).
> >>
> >> This would also make the dispatching in
> >> src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c easier.
> But this
> >> is pure aesthetics, I leave it up to you.
> >>
> >> ---
> >>
> >> src/hotspot/share/jfr/metadata/metadata.xml
> >>
> >> All event definitions have one value, called and labeled
> "Thread". If you
> >> keep them separate, does this field name/label make sense? (I am
> not
> >> fluid
> >> with JMC so I do not know where exactly this label text would
> appear).
> >>
> >> ---
> >>
> >> src/hotspot/share/jfr/periodic/jfrPeriodic.cpp
> >>
> >> GrowableArray<jlong> written(initial_size);
> >> GrowableArray<traceid> thread_ids(initial_size);
> >>
> >> You could probably save one GrowableArray and some append calls by
> >> something like this:
> >>
> >> struct data { jlong num; trace_id tid; }
> >> GrowableArray<data>
> >>
> >> -
> >>
> >> Question, I assume you go thru the trouble of storing the values
> >> temporarily since you do not want to call JfrEvent::commit
> inside the
> >> Threads_lock?
> >>
> >> ---
> >>
> >> src/hotspot/share/runtime/thread.cpp
> >>
> >> Nice addition to the extended thread dump.
> >>
> >> ---
> >>
> >> src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c
> >>
> >> - There may be a number of unnecessary casts:
> >>
> >> e.g. Java_sun_nio_ch_FileDispatcherImpl_read0
> >>
> >> JVM_callNetworkReadBytes(env, (jint) readBytes);
> >>
> >> readBytes is already jint.
> >>
> >> -
> >>
> >> You could reduce the code by factoring out that frequent if-else
> >> statements:
> >>
> >> 87 if (readBytes > 0) {
> >> 88 if (socket) {
> >> 89 JVM_callNetworkReadBytes(env, (jint) readBytes);
> >> 90 } else {
> >> 91 JVM_callFileReadBytes(env, (jint)readBytes);
> >> 92 }
> >> 93 }
> >>
> >> like this
> >>
> >> static void on_io_read(env, jint nread, jboolean socket) {
> >> if (socket) {
> >> JVM_callNetworkReadBytes(env, nread);
> >> } else {
> >> JVM_callFileReadBytes(env, nread);
> >> }
> >> }
> >>
> >> you could even add the n>0 condition to the function.
> >>
> >> -
> >>
> >> Java_sun_nio_ch_FileDispatcherImpl_pwrite0: you truncate the
> return value
> >> of pread64 to 32bit on 64bit platforms.
> >>
> >> ---
> >>
> >>
> src/java.base/windows/native/libnet/DualStackPlainDatagramSocketImpl.c
> >>
> >> Probably philosophical but I would leave peek out of the
> condition. A
> >> read
> >> is a read.
> >>
> >> --
> >>
> >> src/java.base/unix/native/libnet/SocketOutputStream.c
> >>
> >> You could move the call to JVM_callNetworkWriteBytes out of the
> loop (you
> >> did this in other places too).
> >>
> >> --
> >>
> >> src/java.base/windows/native/libnio/ch/DatagramDispatcher.c
> >> src/java.base/windows/native/libnio/ch/FileDispatcherImpl.c
> >>
> >> at various places:
> >>
> >> 52 DWORD read = 0;
> >>
> >> 83 if (read > 0) {
> >> 84 JVM_callNetworkReadBytes(env, read);
> >> 85 }
> >>
> >> DWORD is unsigned, may overflow jint arg? Moot point if you make it
> >> 64bit.
> >>
> >> ----
> >>
> >>
> src/java.base/windows/native/libnet/DualStackPlainDatagramSocketImpl.c
> >>
> >> Java_java_net_DualStackPlainDatagramSocketImpl_socketSend
> >>
> >> 462 if (rv > 0) {
> >> 463 JVM_callFileWriteBytes(env, rv);
> >> 464 }
> >>
> >> Should this be NetworkWriteBytes?
> >>
> >> ---
> >>
> >> src/java.base/windows/native/libnet/SocketOutputStream.c
> >>
> >> Same here.
> >>
> >> ---
> >>
> >>
> src/java.base/windows/native/libnet/TwoStacksPlainDatagramSocketImpl.c
> >>
> >>
> >> - if (sendto(fd, fullPacket, packetBufferLen, 0, addrp,
> >> + if (ret = sendto(fd, fullPacket, packetBufferLen, 0, addrp,
> >> addrlen) == SOCKET_ERROR)
> >> {
> >> NET_ThrowCurrent(env, "Datagram send failed");
> >> }
> >>
> >> + if (ret > 0) {
> >> + JVM_callFileWriteBytes(env, ret);
> >> + }
> >> +
> >>
> >> Same here.
> >>
> >> ---
> >>
> >>
> http://cr.openjdk.java.net/~ghaug/webrevs/8216981/src/java.base/windows/native/libnio/ch/DatagramChannelImpl.c.frames.html
>
> >>
> >>
> >> Java_sun_nio_ch_DatagramChannelImpl_send0
> >>
> >> Smae here.
> >>
> >> ---
> >>
> >> src/java.base/windows/native/libnio/ch/DatagramDispatcher.c
> >> src/java.base/windows/native/libnio/ch/SocketDispatcher.c
> >>
> >>
> >> Same here?
> >>
> >> ---
> >>
> >> src/java.base/windows/native/libnio/ch/WindowsSelectorImpl.c
> >>
> >> + written = send(scoutFd, &byte, 1, 0);
> >> +
> >> + if (written > 0) {
> >> + JVM_callFileWriteBytes(env, written);
> >> + }
> >>
> >> ditto :)
> >>
> >> Thanks for your work!
> >>
> >> Cheers, Thomas
> >>
> >>
> >>
> >>
> >> On Mon, Jan 14, 2019 at 4:59 PM Haug, Gunter
> <gunter.haug at sap.com <mailto:gunter.haug at sap.com>> wrote:
> >>
> >>> Hi All,
> >>>
> >>> Could I please have a review and possibly some opinions on the
> following
> >>> enhancement to JFR and the JDK?
> >>>
> >>> At SAP we have a per thread IO statistic among our supportability
> >>> enhancements which proved to be very helpful for our support
> >>> engineers. It
> >>> might be beneficial for JFR as well and would certainly help us
> to drive
> >>> adoption of OpenJDK.
> >>>
> >>> The basic idea is simple, we have added fields to the thread
> class where
> >>> the number of bytes read and written from/to file and network are
> >>> counted
> >>> in. The newly created JFR events are written periodically as
> for example
> >>> the ThreadAllocationStatistics event already is.
> >>>
> >>> In order to collect the data, we have added hooks to the JDK C
> coding
> >>> calling back into the VM.
> >>>
> >>> I have opened a bug here:
> >>> https://bugs.openjdk.java.net/browse/JDK-8216981
> >>>
> >>> Here is a webrev:
> >>> http://cr.openjdk.java.net/~ghaug/webrevs/8216981
> >>>
> >>> There are no tests yet and the code be a bit nicer in places.
> We will
> >>> work
> >>> on this if/when this feature is deemed acceptable.
> >>>
> >>> Finally, we have an API in our SAP version of the JDK to access
> this
> >>> data
> >>> from a Java application. This is used by many SAP applications
> and I
> >>> think
> >>> we could add an MXBean in a second step, to provide similar
> >>> functionality.
> >>>
> >>> Thanks in advance,
> >>> Gunter
> >>>
> >>>
> >>>
> >>>
> >>>
> >
>
More information about the core-libs-dev
mailing list