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