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
