Re: RFR: 8003322: Add instrumentation points for tracing of I/O calls

[Remi Forax]

Hi Staffan,
in IOTraceAgent,
ASM provides a method named Type.getOpcode(baseOpcode) that allows you
to emit load/store or return depending on the type
(type specialized opcode like ILOAD, ALOAD, etc are in the same order).

// by example for load
mv.visitVarInsn(type.getOpcode(ILOAD), slot);

// and for return
mv.visitVarInsn(type.getOpcode(IRETURN), slot);

also I think your empty private constructor is not valid
because it doesn't call (invokespecial) the constructor of Object.
(I think you haven't a verify error because IOTrace is not
verified because loaded by the bootclassloader).

I also believe that because there is no jump in your code,
there is no need to generate the stack frames
(there is no stack frame).

Rémi

On 11/14/2012 06:35 PM, Staffan Larsen wrote:

Thanks for the detailed review, Alan. Comments inline.

On 14 nov 2012, at 13:50, Alan Bateman <alan.bate...@oracle.com> wrote:

On 13/11/2012 10:16, Staffan Larsen wrote:

This is a request for review for adding tracing to I/O calls. For now, this is 
an empty infrastructure intended to enable diagnosing/tracing of i/o calls. A 
user of the API can register a callback for read and write operations on 
sockets and files. It does not (yet) cover asynchronous i/o calls. When not 
used, the implementation should add a minimum of overhead. To provide useful 
information to the user, FileInputStream, FileOutputStream and RandomAccessFile 
have been modified to keep track of the path they operate on (when available).

Webrev: http://cr.openjdk.java.net/~sla/8003322/webrev.00/

Thanks for the update. Do you have any updated performance data to share (just 
to confirm that the updated implementation doesn't have any real impact)?

While I haven't been able to measure an impact myself, I want to confirm this 
with runs from the performance team. I'll get back as soon as I have something 
to share.

Anyway, I took a pass over the new webrev.

I'm not sure that passing a value of 0 for errors to xxEnd is the best 
approach, particularly if this is ever extended to non-blocking I/O. Also I 
think there are a few inconsistencies with respect to EOF -- eg: in 
FileInputStream then read() will call the hook with 0 at EOF whereas the other 
read methods will call the hook with -1 at EOF.  In FileChannelImpl then some 
places use normalize, some not.

Thanks for catching these inconsistencies, I have fixed them.

I guess the main question is whether the agent needs to distinguish I/O errors 
from EOF and 0 bytes (the latter is assuming this may be extended to non-blocking 
I/O). It may be that you need to use -2 or anything < -1 to distinguish all 
cases.

This one is hard. As you say, it would be great to differentiate between 0 
bytes, EOF and Exceptions. The first two are quite easy as I could make -1 mean 
EOF. Exceptions are harder since I don't really know if there was an exception 
from where the xxEnd() method is called now (typically a finally clause). 
Adding a catch clause and calling xxEnd() from there would solve it, but make 
the code more complicated. Hard to tell if the extra code complexity is worth 
it.

Minor nit but there is a bit of inconsistency with the variables names, usage of 
"v" in RandomAccessFile for example whereas FIS/FOS have bytesRead and 
bytesWritten.

I change v to bytesRead or bytesWritten as appropriate.

Thanks for adding javadoc to IoTrace. One suggestion is to include a big 
warning that the hooks may be called while holding low-level locks in the 
implementation and so great care must be taken, any synchronization or 
interaction with other threads could easily deadlock the VM.

I have added this.

I skimmed over the tests (not a detailed review) and they look reasonable. You 
might need to check the copyright headers as it looks like at least one of the 
tests has the GPL+Classpath exception whereas we normally use just the GPL 
header on tests.

Fixed.

Also good to ensure that there is @bug tag on the tests to link it to 8003322.

Added.

In ioTraceTest.sh I see "cd ${PWD}" that I didn't quite get.

I do a few "cd" to different places to compile and create the jar, I then 
wanted to go back to the original directory to execute the test.

Do you think these tests will be reliable when running without an images build 
(meaning raw classes files on the system)? Just wondering if expectFileRead 
might fail due to I/O caused by class loading.

I have been running them without an image build with no problem, but I see what 
you mean. If this turns out to be a problem, then some classes may have to be 
pre-loaded (such as FileInputStream, FileOutputStream, FileChannel*, 
ByteBuffer).

That's all I have on the detailed review.

Thanks!
/Staffan

As you mentioned there is still have the substantive issue as to whether it's 
open season on sun.misc.IoTrace*. Ignoring Unsafe (we know this needs to be 
standardized or a standard alternative introduced), then nothing outside of the 
JDK should be using sun.* classes directly.

-Alan