Improving perf of FileChannel.transferTo() on Windows

[Alan Bateman]

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




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