Use file channel in smoosher to minimize copy overheads

[navis]

Current write(ByteBuffer buffer) implementation of smoother is something like this,

Channels.newChannel(out).write(buffer)

And agagin,

public static ReadableByteChannel newChannel(final InputStream in) {
        if (in instanceof FileInputStream &&
            FileInputStream.class.equals(in.getClass())) {
            return ((FileInputStream)in).getChannel();
        }
        return new ReadableByteChannelImpl(in);
    }

but because out is a BufferedOuputStream, this call always makes wrapper which copies ByteBuffer handed over to byte[], negating good intention of using it. I think it would be better to use FileChannel directly.

[xvrl]

@navis any reason this is changing?

[navis]

it was buffered stream before but is not, making that difference. I've implemented buffered channel and it's restored as before.

[drcrallen]

if flush throws IOException, does close() still need called?

[navis]

hm.. good.

[leventov]

Double buffering here. Channels.newChannel() returns a buffered channel, and then ByteStreams.copy() uses a buffer internally. Made google/guava#2559 about this but it's faster and easier to fix internally in the meantime

[navis]

Yes, I've neglected this because this is not used in Druid. But seemed good this to be fixed.

[leventov]

Agree, could be a separate PR. Also found this idiom (ByteStream.copy(Channels.newChannel(), ...) is used in Druid in several places, so it's more a Druid's issue than java-util's

[leventov]

@navis do you have time to address the comments? I can do this myself and make a different PR based on your PR, but if it is merged with squash your authorship will be lost.

[leventov]

@navis could you please make two things?

1. Remove channel buffering: it causes deterministicFileUnmapping() test to fail now, because it 1) allocates a direct buffer 2) duplicates mapped buffers.
2. Close FOS in Outer as suggested here: Use file channel in smoosher to minimize copy overheads #46 (comment)

[leventov]

And after those changes SmooshedFileMapperTest.testBehaviorWhenReportedSizesSmall() will fail. The last assertion in this test, namely Assert.assertEquals(0, files[0].length()); should simply be removed because it seems to rely on FileSmoosher implementation details. Having or not some bytes written to the last smooshed file after ISE is thrown doesn't seem to be an important difference, because metadata is not written anyway and this smooshed dir couldn't be read later.

[leventov]

@navis do you have capacity for doing this?

[navis]

@leventov Addressed comments

[leventov]

@navis Thanks!