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

[Staffan Larsen]

Thanks for the detailed review, Alan. Comments inline.

On 14 nov 2012, at 13:50, Alan Bateman <Alan.Bateman at 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
> 
> 
> 
>