-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8268435: (ch) ChannelInputStream could override readAllBytes #5645
Conversation
👋 Welcome back bpb! A progress list of the required criteria for merging this PR into |
Throughput measured without the change for various stream lengths was
and with the change in place was
In effect, degraded performance for very small lengths is traded off against better performance for larger lengths. Note however that the Unfortunately this is mostly code duplication from |
Webrevs
|
The overrides of readAllBytes and readNBytes in ChannelInputStream look okay. |
The third commit, version 02, adds a TestNG test which replaces the previously expanded |
import org.testng.annotations.DataProvider; | ||
import org.testng.annotations.Test; | ||
|
||
public class ChannelInputStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests for Channels API are in java/nio/channels/Channels and we should try keep them in the same tree if possible. If the test is focused on readXXXBytes then we should come up with a better name for the test, maybe "ReadXBytesTest" rather than "ChannelInputStream".
} | ||
} finally { | ||
file.delete(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier to maintain, and allow us to add more cases, if we split this up so there are separate tests for empty files, OOME, etc.
Are you planning to update the test cover the case that the initial position is > 0?
Has the setup been copied from an older test, I'm wondering why it needs to use File and FileChannel::getChannel as you should be able to use FileChannel.open.
} | ||
} finally { | ||
file.delete(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier to maintain and add new cases if this one were split up too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, the test is much better now.
@LanceAndersen Do you have any comments on this one?
@bplb This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 209 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Brian,
Looks good overall. A couple minor suggestions for you to consider but I am good to go either way.
|
||
private static final int BIG_LENGTH = ArraysSupport.SOFT_MAX_ARRAY_LENGTH; | ||
private static final long HUGE_LENGTH = Integer.MAX_VALUE + 27L; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps consider adding a comment for the above length constants
assertNotNull(bytes); | ||
assertEquals(bytes.length, (long)length); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been suggested in past PR reviews (including a few of mine) that we use assertThrows(). vs the expectedException annotation element. Something you might want to consider using here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Brian for the last updates. Looks good. :-)
/integrate |
Going to push as commit 0786d8b.
Your commit was automatically rebased without conflicts. |
This introduced JDK-8274780. Fixed proposed via #5824. |
This change would override
readAllBytes()
andreadNBytes(int)
inChannelInputStream
thereby improving performance for all but smaller streams.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5645/head:pull/5645
$ git checkout pull/5645
Update a local copy of the PR:
$ git checkout pull/5645
$ git pull https://git.openjdk.java.net/jdk pull/5645/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5645
View PR using the GUI difftool:
$ git pr show -t 5645
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5645.diff