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

[Staffan Larsen]

I now have some micro-benchmark numbers on windows and linux (the solaris runs are not complete yet). While doing these runs we initially saw a regression on the file reading benchmarks. Investigation showed that the compiler was not able to inline the IoTrace.xxBegin() and IoTrace.xxEnd() methods because the IoTraceContext class in the signature had not yet been loaded. This forced me to go back to just an Object in the signatures for these methods which allowed them to be inlined and performance restored. 

The newest version of the code is here: http://cr.openjdk.java.net/~sla/8003322/webrev.01/
Results of the benchmarks are here: http://cr.openjdk.java.net/~sla/8003322/webrev.01/Table.htm
A copy of the microbenchmarks is here: http://cr.openjdk.java.net/~sla/iotrace/io-micros/

The microbenchmarks uses a harness that has not yet been open sourced, but I still think the code can be read and understood. It seems like the socket benchmarks show a much larger variance in the results than the file benchmarks. 

Thanks,
/Staffan

On 14 nov 2012, at 18:35, Staffan Larsen <staffan.larsen at oracle.com> wrote:

> Thanks for the detailed review, Alan. Comments inline.
> 
> On 14 nov 2012, at 13:50, Alan Bateman <Alan.Bateman at 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
>> 
>> 
>> 
>> 
>