Preliminary review: Adding tracing of I/O calls

[Staffan Larsen]

Thanks Alan. Some comments inline.

On 2 nov 2012, at 23:21, Alan Bateman <Alan.Bateman at 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.