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

[Alan Bateman]

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

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.

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.

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.

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 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. Also good to ensure that 
there is @bug tag on the tests to link it to 8003322. In ioTraceTest.sh 
I see "cd ${PWD}" that I didn't quite get.

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.

That's all I have on the detailed review. 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