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

[Remi Forax]

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