RFR: 8278268 - (ch) InputStream returned by Channels.newInputStream should have fast path for FileChannel targets [v18]

[Markus KARG]

On Wed, 1 Mar 2023 01:37:42 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Markus KARG has refreshed the contents of this pull request, and previous commits have been removed. Incremental views are not available.
>
> In the test classes `TransferTo` and `TransferTo2`, the methods `testNullPointerException` and `testStreamContents` are identical aside from expecting different `DataProvider`s. The contents of the methods could be abstracted out to common methods called by different `Test`s which expect different `DataProvider`s.
> 
> The test classes `TransferTo_2GB_transferFrom` and `TransferTo_2GB_transferTo` comprise roughly 120 lines of code each, but a diff of their contents reveals little in the way of substantive differences aside from jtreg tags, comments, and names:
> 
> 96,99c95,97
> <                 // performing actual transfer, effectively by multiple invocations of
> <                 // FileChannel.transferFrom(ReadableByteChannel)
> <                 try (InputStream inputStream = Channels.newInputStream(Channels.newChannel(
> <                         new BufferedInputStream(Files.newInputStream(sourceFile))));
> ---
>>                 // perform actual transfer, effectively by multiple invocations
>>                 // of FileChannel.transferTo(WritableByteChannel)
>>                 try (InputStream inputStream = Channels.newInputStream(FileChannel.open(sourceFile));
> 
> Perhaps something along these lines might work:
> * rename `TransferToBase` to for example to `TransferBase` or `TransferFromToBase`;
> * rename `TransferTo2` to `TransferFrom`;
> * rename `TransferTo_2GB_transferX` classes to `TransferX_2GB`;
> * refactor common code to `TransferBase` along the lines described above;
> * refactor child classes as needed to use the common code.
> 
> Assuming the foregoing makes sense, there would remain a total of four tests. It might be possible also to merge `TransferFrom` and `TransferTo` into a single test, but I think the two tests `*_2GB*` should remain separate as they likely each have a sufficiently long run time.

@bplb Before I stark to refactor anything, it makes sense to briefly discuss your recent proposal:
> In the test classes `TransferTo` and `TransferTo2`, the methods `testNullPointerException` and `testStreamContents` are identical aside from expecting different `DataProvider`s. The contents of the methods could be abstracted out to common methods called by different `Test`s which expect different `DataProvider`s.

This is correct and I have no objections to that, but on the other hand, there is no need to keep the separate `TransferTo2`  class separated, as the reason for the existence of `TransferTo2` was a bug somewhere else in OpenJDK that does not
exist anymore (for reference, the bug was in the Pipe implementation on Windows). Do that fact, Lance proposed to *merge* these files:
> I haven't gone through this in detail, but we want to merge The remaining TransferTo2 tests into TransferTo. 

So can you please briefly **confirm** that I *shall not* merge `TransferTo2` into `TransferTo` as *Lance* proposed (hence `TransferTo2` will not be a *separate* class anymore) but **instead** I *only shall* abstract `testNullPointerException` and `testStreamContents` as *you* proposed (hence keep `TransferTo2` as a *separate* class)? Thanks.

>...I think the two tests `*_2GB*` should remain separate as they likely each have a sufficiently long run time.

I confirm that we had intentionally kept these two tests in separate classes due to the long run time of each, so I agree to keep them *as separate* classes, but I will reduce *duplicate* code.

I need to mention that the **file names** *should not* be changed according to you proposed, as `*_transferFrom` is actually *not* testing the method `TransferFrom`, but it is testing the method `TransferTo`'s implementation which *internally uses* `transferFrom`. Hence it would be wrong w.r.t to the general test file naming convention to rename the test to `TransferFrom`. 

So can you please briefly **confirm** that I am correct with this objection and the 2GB files *should not* be renamed?

-------------

PR: https://git.openjdk.org/jdk/pull/6711