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 <fo...@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.bate...@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 >>> >>> >>> >>> >
