[RFR]: Per thread IO statistics in JFR
Alan Bateman
Alan.Bateman at oracle.com
Wed Jan 16 14:59:43 UTC 2019
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. 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 hotspot-jfr-dev
mailing list