[RFR]: Per thread IO statistics in JFR
David Holmes
david.holmes at oracle.com
Thu Jan 17 00:23:01 UTC 2019
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.
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 hotspot-jfr-dev
mailing list