InputStreamMessageReader performance and logic issues

[ghost]

While looking to obtain a bit more performance, I noticed a few things about InputStreamMessageReader that could be addressed. See patch file in comment below. These include some minor issues, in no particular order:

• Assign values to final instance variables at declaration site (not in the constructor) to avoid a double assignment (the JVM will init member variables to null/0/false before constructor is called).
• Limit variables to narrowest scope possible (e.g., rv).
• Make variables final.
• Remove superfluous catch blocks.
• Declare and assign variables as close to first usage as possible (e.g., endian, type, protocol version).
• Avoid use of modulus for powers of two (super minor micro optimization; javac doesn't do this one, yet, IIRC).
• Eliminate single-use variables (such as bodylen).
• Use finally block to avoid duplication of variable resets.

As well as some bugs both actual and theoretical:

• rv must use an inequality relation (< 0) because .read can return multiple negative values. (Aside, usually compares to 0 are faster than non-zero, at least on x86 architecture.)
• inputChannel, technically, could become null while readMessage is executing; using a local instance avoids that possibility. (This is only a problem if the reader is used from multiple threads; adding a safeguard doesn't hurt.)

Here's a screenshot of "D-Bus 4.1.1" (from main branch, no changes to the reader):

Here's a screenshot of "D-Bus 4.1.2" (patch file applied to main branch):

If I'm reading the the profiling information correctly, the changes reduce the total CPU usage by a piddly 800 ms every 200,000 ms for a very modest 0.4% overall improvement.

Attached are the .nps files that can be loaded into VisualVM 2.1.4:

• dbus-visualvm.zip

[ghost]

Patch file for the changes:

• InputStreamMessageReader.patch.txt

[hypfvieh]

I've used some of your improvements and added some more.

Regarding the return of channel.read(), the javadoc says:

Returns: The number of bytes read, possibly zero, or -1 if the channel has reached end-of-stream

So checking for rv == -1 should work. Any other error on read() is signaled by an exception (IOException in most cases). However, using rv < 0 will also work, so I kept that change.

[ghost]

So checking for rv == -1 should work.

Here's what I found for JDK 8:

• InputStreamMessageReader calls SocketChannel::read
• SocketChannel::read calls SocketChannelImpl::read
• SocketChannelImpl::read calls IOUtil.read
• IOUtil.read calls IOUtil::readIntoNativeBuffer
• IOUtil::readIntoNativeBuffer calls NativeDispatcher::read
• NativeDispatcher::read calls into one of three factory methods, presumably SocketDispatcher::read
• SocketDispatcher::read calls SocketDispatcher::read0 (native method)
• SocketDispatcher::read0 directs to Java_sun_nio_ch_SocketDispatcher_read0 (JNI call)
• Java_sun_nio_ch_SocketDispatcher_read0 can return IOS_UNAVAILABLE and IOS_THROWN.

Presumably these C-level values are mapped to the following codes in IOStatus,java:

    @Native public static final int UNAVAILABLE = -2;      // Nothing available (non-blocking)
    @Native public static final int THROWN = -5;           // Exception thrown in JNI code

It looks like UNAVAILABLE is normalized to 0 and THROWN results in an exception, as you said.

Thanks for applying the patch!