Preliminary review: Adding tracing of I/O calls

[Aleksey Shipilev]

On 11/03/2012 01:15 AM, Mandy Chung wrote:
> On 11/2/2012 1:47 PM, Staffan Larsen wrote:
>> On 2 nov 2012, at 21:12, Mandy Chung<mandy.chung at oracle.com>  wrote:
>>
>>> The *Begin() methods return a "handle" that will be passed to the
>>> *End() methods.  Have you considered to define a type for it rather
>>> than Object?
>> Something like an empty interface, just to signal the intent?
> 
> Yes I think so.  A marker interface would suffice.
> 
>>> Do you have any performance measurement result that you can share?
>> I don't yet have any specific numbers - I'll try to get some. The
>> testing I have done indicates that the overhead is negligible. But it
>> would be good to confirm this.
> 
> refworkload is easy to run. You probably have talked with the
> performance team. You can find out from them which benchmarks, if they
> have any, are applicable for IO instrumentation.

Performance team here.

There are virtually no benchmarks against I/O per se. Looking at the
patch, I would think anything doing intensive network I/O would help to
quantify the change, which boils down to SPECjbb2012, SPECjEnterprise,
and Volano. First two would be hard to run, and the third has terrible
run-to-run variance, so you will probably have to quantify the changes
with microbenchmarks. It should be easy enough to construct with our
micro harness (still not available in OpenJDK). Contact me internally if
you want to get that route.

General patch review: I do have the preoccupation against interferenced
tracing code, and while appreciating the intent for tracing patch, we
need to look for performance penalties. The rule of thumb is that
HotSpot will optimize away the code guarded by static final flag (or, as
Remi points out with jsr292 magic). Doing the calls hoping for HotSpot
to inline and figure out the absence of useful work there is not working
reliably either. Inline conditionals will cost something if the tracing
method is not inlined.

Hence, I would rather recommend to switch the uses like this:

    public int read() throws IOException {
        Object traceHandle = IoTrace.fileReadBegin(path);
        int b = read0();
        IoTrace.fileReadEnd(traceHandle, b == -1 ? -1 : 1);
        return b;
    }

...to something more like:

    public int read() throws IOException {
        if (IoTrace.ENABLED) {
            Object traceHandle = IoTrace.fileReadBegin(path);
            int b = read0();
            IoTrace.fileReadEnd(traceHandle, b == -1 ? -1 : 1);
            return b;
        } else {
            return read0();
        }
    }

where

  class IoTrace {
     public static final boolean ENABLED =
System.getProperty("java.io.trace");
  }

...which will demote the flexibility of setListener(), but have much
lower runtime overhead. This should be confirmed by microbenchmarking
anyway.

-Aleksey.