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

[Staffan Larsen]

Hi Rémi,

Thanks for your suggestions, they made the code much simpler - and more correct!

/Staffan

On 15 nov 2012, at 09:04, Remi Forax <forax at univ-mlv.fr> wrote:

> Hi Staffan,
> in IOTraceAgent,
> ASM provides a method named Type.getOpcode(baseOpcode) that allows you
> to emit load/store or return depending on the type
> (type specialized opcode like ILOAD, ALOAD, etc are in the same order).
> 
> // by example for load
> mv.visitVarInsn(type.getOpcode(ILOAD), slot);
> 
> // and for return
> mv.visitVarInsn(type.getOpcode(IRETURN), slot);
> 
> also I think your empty private constructor is not valid
> because it doesn't call (invokespecial) the constructor of Object.
> (I think you haven't a verify error because IOTrace is not
> verified because loaded by the bootclassloader).
> 
> I also believe that because there is no jump in your code,
> there is no need to generate the stack frames
> (there is no stack frame).
> 
> Rémi
> 
> On 11/14/2012 06:35 PM, Staffan Larsen 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
>>> 
>>> 
>>> 
>>> 
>