Improving perf of FileChannel.transferTo() on Windows

[Martin Sawicki (MS OPEN TECH)]

From: Alan Bateman [mailto:Alan.Bateman at oracle.com]


On 14/11/2014 16:51, Kirk Shoop (MS OPEN TECH) wrote:
From: Alan Bateman [mailto:Alan.Bateman at oracle.com]


I think this looks much better.
Great!

One thing that needs a closer look is the loop around TransmitFile. If it fails on the second or subsequent call then transferTo will fail after already sending transmitting bytes. For transferTo then it's okay to complete without having transmitted all bytes and maybe it would be better to just cap it at 2GB. Any robust code using transferTo should check the return value. (BTW: You can use java_lang_Integer_MAX_VALUE to avoid the repeating the constant).
Good point, Valery removed the loop.

In transferToDirectly then I don't think you need to check if the target is a SelectableChannel if you move call to nd.canTransferToDirectly to just above where it handles SelChImpl.
Done.

One suggestion is to top transferToDirectlyEnabled and instead jave canTransferToDirectly return true then the property is enabled and the target change is SelectableChannel configured blocking.
Done.

A minor comment is that transferToDirectlyChangesChannelPosition is a bit inconsistent with methods like needPositionLock so you might be able to come up with something a bit closer to that, transferDirectNeedsPositionLock maybe?
Valery chose 'transferToDirectlyNeedsPositionRestoring', but this can be changed.

While looking at this, I wondered, could the position lock and position manipulation in Java be skipped altogether if 'Java_sun_nio_ch_FileChannelImpl_transferTo0' used 'Java_sun_nio_ch_FileChannelImpl_position0' to restore the position to the original point?

A few style/formatting issues that we can sort out later, otherwise the approach looks okay to me.
Happy to take those changes anytime.



Sorry for the delay getting back to you on this, I've been busy with other things.

I think the updated version looks good and you've addressed the issues that I was concerned about.  One other potential issue that came to mind later is that the blocking mode of the SocketChannel could change to non-blocking at or around the time that transferTo is called. It would be possible to restructure this to hold the blockingLock but I think it makes the locking more complicated. Worst case with this "bug" is that transferTo blocks at around the time the blocking mode is changed to non-blocking. There is a bigger (and longstanding) issue in this area related to async close of the target channel during a transfer and if/when that gets looked at then I think it is the time to address the blocking lock issue too.

I had to re-base your patch to get it working with the current jdk9/dev tip, that is only because of other recent changes in this area. I tweaked some minor formatting issues and used the opportunity to replace the imports that use wildcards. I don't have a strong opinion on the method to determine if the position changes except that transferToDirectlyNeedsPositionLock is a bit more consistent with the current naming. Here is the updated patch (only minor edits as I said):

   http://cr.openjdk.java.net/~alanb/8064407/webrev/

I've run this patch through our build+test system to make sure that it works on the 7+ platforms that we normally care about and I don't see any issues. I've also run the jdk_nio tests with the property enabled so that all tests using transferTo use TransmitFile when the channel is configured blocking.

Let me know if you are okay with this?

-Alan
Kirk has been sick, but the answer is Yes.
Thank you.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20141205/dc6e3a37/attachment-0001.html>