Thanks Alan. Some comments inline. On 2 nov 2012, at 23:21, Alan Bateman <alan.bate...@oracle.com> wrote:
> On 02/11/2012 18:36, Staffan Larsen wrote: >> This is a preliminary review request for adding an API for tracing 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 listener >> and get callbacks 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/iotrace/webrev.00/ >> >> Feedback is most welcome, >> /Staffan >> > Part of me is a somewhat disappointed to see hooks going into the I/O paths, > but I understand why it needs to be done. I see the mails that getting some > performance figures on the overhead and that would be good to have. Yes, it worries me a bit, too. I'll see what the microbenchmarks say. > I think IoTrace/IoTraceListener needs to have some javadoc. I suggest this > because it's impossible to implement IoTraceListener (even in the JDK) > without some documentation as to how it is used. I see there is a block > comment in IoTraceListener but there are other things that an implementer > needs to know, particularly as to possible behavior when there is an I/O > error. Looking at the usages then it looks like in *End might not be called, > in other cases it can be called with 0 when there is an error. Wil fix. > I also see mails about IoTrace.listener needing to be volatile, that would be > good to do. > > I also think it would be useful to have a basic sanity test of the hooks. I > realize there will be product usage elsewhere but we should have something > simple in the jdk repository too. Will fix. > In FileInputStream, FileOutputStream and FileChannelImpl then the comment on > path is that it is null "if there is no file" but I think this should say > that the stream (or parent stream in the case of a file channel) is created > with a file descriptor. Yes. > In SocketChannelImpl then if the channel is configured non-blocking > socketReadEnd will be called without a socketReadBegin. If this is intended > then it will be something for the IoTrace/IoTraceListener javadoc. This is an error. The socketReadEnd() call should be guarded the same way socketReadBegin() is. > I realize the focus is blocking I/O for now but one thing to know is that > timed read operations on socket adapters (the socket obtained by calling > SocketChannel's socket method) configures the socket channel to be > non-blocking so this means that this I/O will not be observed by the > IoTraceListener. I need some help to understand which call path you are referring to here. I see SocketChannelImpl.socket() returning a SocketAdapter, but I don't see how/if this is Socket is configured to be non-blocking. > In both FileChannelImpl and SocketChannelImpl then normalize will now be > called twice on each return status, I don't expect this will be observable > from a performance perspective. Yes, I would be surprised if this was observable. I could rework the code so it's only called once. > > SolarisUserDefinedFileAttributeView - this is I/O on a file's extended > attributes rather than its contents, it might not interesting to the > IoTraceListener. Hard to tell if it will be interesting or not. If there is i/o related to the file, it is probably of interest when diagnosing problems. I also don't know how to exclude this information in a simple way. > UnixChannelFactory L137, this line is getting long, might be better to go > into a second line. Will fix. > WindowsChannelFactory - one thing to be aware of is newFileChannel will also > be called when open named streams so pathForWindows won't be the name of a > file that you see on the file system. I don't know if this is interesting > here or not, it should be rare. Sounds like the name of the stream would also be of interest to anyone tracing/diagnosing. /Staffan > That's all I have. > > -Alan.
