[RFR]: Per thread IO statistics in JFR

[Thomas Stüfe]

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
>
>
>
>
>