8259956: jdk.jfr.internal.ChunkInputStream#available should return the sum of remaining available bytes #2138
Conversation
…e sum of remaining available bytes
|
/label add hotspot-jfr |
|
👋 Welcome back ddong! A progress list of the required criteria for merging this PR into |
|
@D-D-H |
Webrevs
|
|
Hi Denghui, Looks reasonable, but I will do a proper review of this and your other PRs once everything is set for JDK 16. Erik |
okay, thanks :) |
|
Not sure things should be changed. Javadoc for Inputstream::available() says: "Returns an estimate of the number of bytes that can be read (or skipped over) from this input stream without blocking by the next invocation of a method for this input stream." How often the stream will block depends on the underlying BufferInputStream, so it seems correct to just delegate. Purpose of the available() method is not to return the total. |
In my humble opinion, Most users always expect this method to return the total readable size, and this is achievable for this method, although the java doc does not force the implementer to do so, what do you think? |
|
There is a method for finding the size of a recording, Recording::getSize(). It returns a long, which means it will work in cases where the recording is larger than 2 GB. I would recommend using it if you want to know the size in a reliable way. That said, it will probably not hurt changing so that available() return the size of what is left unread on disk, potentially in memory in the future, or from an ongoing stream in a later release. From what I can see, Files.newInputStream(...).available() returns the full file size. This is also what java.io.FileInputStream does and BufferInputStream delegates to available() in the wrapped stream. java.io.SequenceStream on the other hand works like ChunkInputStream today. One problem with the implementation is that you iterate over all chunks with every call to available(). It would be nice to avoid this. I also wonder if you considered making total a long value and check for overflow once and near the return statement? |
But if I get the data of a period of time in the past through Recoding::getStream, I cannot know its size.
I used "unstreamedSize" to cache the size.
yes, good idea! |
| r.enable(EventNames.JavaThreadStatistics) | ||
| .withPeriod(Duration.ofMillis(100)); | ||
| r.start(); | ||
| File repository = Path.of(System.getProperty("jdk.jfr.repository")).toFile(); |
There was a problem hiding this comment.
I would prefer a simpler and more deterministic test that executes quickly and doesn't involve timing or other events (to reduce false positives).
How about this? (haven't tried it).
public static void main(String... args) throws Exception {
try (Recording r = new Recording()) {
r.start();
try (Recording s = new Recording()) {
s.start(); // rotate
s.stop(); // rotate
}
r.stop();
try (InputStream is = r.getStream(null, null)) {
int left = is.available();
if (left != r.getSize()) {
String msg = "Expected IS::available() " + left;
msg += " to equal recording size " + r.getSize();
throw new Exception(msg);
}
while (is.read() != -1) {
left--;
int available = is.available();
if (available != left) {
String msg = "Expected IS::available() to return " + left;
msg += ", got " + available;
throw new Exception(msg);
}
}
}
}
}
|
@D-D-H 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 295 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@egahlin) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
/sponsor To reduce noise on the mailing list, you may consider pushing all changes at once, or remove the hotspot-jfr label temporarily, so notifications are not sent when you are updating the PR. Thanks |
openjdk
bot
added
integrated
openjdk
bot
removed
ready
|
@egahlin @D-D-H Since your change was applied there have been 317 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit e8ad8b3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Could I have a review of this small fix?
In the current implementation, ChunkInputStream#available only returns the available size of the current stream, which is strange for the user.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2138/head:pull/2138$ git checkout pull/2138