[RFR]: Per thread IO statistics in JFR
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Jan 17 07:23:58 UTC 2019
On Thu, Jan 17, 2019 at 1:23 AM David Holmes <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).
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.
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.
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>
> 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