Re: RFR 4358774: Add null InputStream and OutputStream
Hi Roger, I agree. It does not seem that whatever performance improvement might accrue from not using the Objects methods is offset by the increased code readability in addition to mitigating the risks mentioned in [1]. I have reinstated this approach in http://cr.openjdk.java.net/~bpb/4358774/webrev.02/. Thanks, Brian On Dec 7, 2017, at 2:04 PM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
For checking indices, I think you should leverage the work done for java.util.Objects.checkFromIndexSize(...) as optimized for this purpose in 8135248. That was extensively designed and reviewed for optimal performance.
Regards, Roger
On 07/12/2017 14:55, Brian Burkhalter wrote:
Hi Roger,
I agree. It does not seem that whatever performance improvement might accrue from not using the Objects methods is offset by the increased code readability in addition to mitigating the risks mentioned in [1]. I have reinstated this approach in http://cr.openjdk.java.net/~bpb/4358774/webrev.02/.
Why the Objects.requireNonNull(b); should be called before Objects.checkFromIndexSize(off, len, b.length); since the second call will gen NPW at b.length anyway?
Thanks,
Brian
On Dec 7, 2017, at 2:04 PM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
For checking indices, I think you should leverage the work done for java.util.Objects.checkFromIndexSize(...) as optimized for this purpose in 8135248. That was extensively designed and reviewed for optimal performance.
Regards, Roger
-- Best regards, Sergey.
Hi Sergey, On Dec 7, 2017, at 3:14 PM, Sergey Bylokhov <Sergey.Bylokhov@oracle.com> wrote:
Why the Objects.requireNonNull(b); should be called before Objects.checkFromIndexSize(off, len, b.length); since the second call will gen NPW at b.length anyway?
Good point. I’ll update after I see whether any other updates will be needed. I think this situation probably obtains in some other files as well. Thanks, Brian
On 07/12/2017 22:55, Brian Burkhalter wrote:
Hi Roger,
I agree. It does not seem that whatever performance improvement might accrue from not using the Objects methods is offset by the increased code readability in addition to mitigating the risks mentioned in [1]. I have reinstated this approach in http://cr.openjdk.java.net/~bpb/4358774/webrev.02/.
I read through the javadoc again and I think it looks good. On the implementation then Sergey has a point, the requireNonNull isn't needed to check b when b.length is used on the next line. One other nit is that the "overridden for efficiency" comment between @Override and the method declaration is a distraction. I don't think the comment is needed or just put it on the same line or above the @Overview so it doesn't get in the way. The NullXXX tests can be converted to TestNG unit tests if you have cycles. -Alan
On Dec 8, 2017, at 4:39 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
I read through the javadoc again and I think it looks good.
On the implementation then Sergey has a point, the requireNonNull isn't needed to check b when b.length is used on the next line.
Already made this change locally.
One other nit is that the "overridden for efficiency" comment between @Override and the method declaration is a distraction. I don't think the comment is needed or just put it on the same line or above the @Overview so it doesn't get in the way.
The NullXXX tests can be converted to TestNG unit tests if you have cycles.
I’ll make the above two changes and re-post later today. Thanks, Brian
On Dec 8, 2017, at 8:29 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Dec 8, 2017, at 4:39 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
I read through the javadoc again and I think it looks good.
On the implementation then Sergey has a point, the requireNonNull isn't needed to check b when b.length is used on the next line.
Already made this change locally.
One other nit is that the "overridden for efficiency" comment between @Override and the method declaration is a distraction. I don't think the comment is needed or just put it on the same line or above the @Overview so it doesn't get in the way.
The NullXXX tests can be converted to TestNG unit tests if you have cycles.
I’ll make the above two changes and re-post later today.
All previous suggested changes have been made in http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ except for the possible change of name for the IS and OS nullStream() methods which awaits consensus. Thanks, Brian
Hi, Brian. On 08/12/2017 15:12, Brian Burkhalter wrote:
All previous suggested changes have been made in
One more issue that according to the spec the new method read(byte[], int, int) should throw an exception if the stream was closed, but as far as I understand it can return "0" if "len=0" even if the stream was closed before: 99 @Override 100 public int read(byte[] b, int off, int len) 101 Objects.checkFromIndexSize(off, len, b.length); 102 if (len == 0) { 103 return 0; 104 } 105 ensureOpen(); 106 return -1; 107 } -- Best regards, Sergey.
Hi Sergey, On Dec 8, 2017, at 4:34 PM, Sergey Bylokhov <Sergey.Bylokhov@oracle.com> wrote:
One more issue that according to the spec the new method read(byte[], int, int) should throw an exception if the stream was closed, but as far as I understand it can return "0" if "len=0" even if the stream was closed before: 99 @Override 100 public int read(byte[] b, int off, int len) 101 Objects.checkFromIndexSize(off, len, b.length); 102 if (len == 0) { 103 return 0; 104 } 105 ensureOpen(); 106 return -1; 107 }
I agree it looks strange but it is intentional as it matches the existing InputStream.read(byte[],int,in) [1]. (I will remove line 167 as part of this patch.) Note that the IOE for the stream being closed would not be thrown in the current code until line 173. Thanks, Brian [1] http://hg.openjdk.java.net/jdk/jdk/file/dd5157f363ab/src/java.base/share/cla...
On 08/12/2017 16:49, Brian Burkhalter wrote:
I agree it looks strange but it is intentional as it matches the existing InputStream.read(byte[],int,in) [1]. (I will remove line 167 as part of this patch.) Note that the IOE for the stream being closed would not be thrown in the current code until line 173.
Yes it is match the behavior, but both have a different specifications: In the old methods there is a notion: "<p> If <code>len</code> is zero, then no bytes are read and <code>0</code> is returned;" but in the new method we have only one strong statement: "After the stream has been closed, these methods all throw {@code IOException}." -- Best regards, Sergey.
On Dec 8, 2017, at 5:06 PM, Sergey Bylokhov <Sergey.Bylokhov@oracle.com> wrote:
On 08/12/2017 16:49, Brian Burkhalter wrote:
I agree it looks strange but it is intentional as it matches the existing InputStream.read(byte[],int,in) [1]. (I will remove line 167 as part of this patch.) Note that the IOE for the stream being closed would not be thrown in the current code until line 173.
Yes it is match the behavior, but both have a different specifications: In the old methods there is a notion: "<p> If <code>len</code> is zero, then no bytes are read and <code>0</code> is returned;" but in the new method we have only one strong statement: "After the stream has been closed, these methods all throw {@code IOException}."
Indeed you are correct. As we are trying to match the behavior I think the spec will need to be tweaked a little. The problem is that there is not a method-by-method specification in this case. This will need to be reexamined. Thanks, Brian
Hi, Brian On 12/8/17 3:12 PM, Brian Burkhalter wrote:
The changes look good to me. I just noticed a couple small things in the test: test/jdk/java/io/InputStream/NullInputStream.java 109 @Test(groups = "open") 110 public static void testreadNBytes() { 111 try { 112 assertEquals(0, openStream.readNBytes(new byte[1], 0, 1), 113 "readNBytes(byte[],int,int) != -1"); Line 113 should read "... != 0", yes? 129 @Test(groups = "open") 130 public static void testSkip() { 131 try { 132 assertEquals(0, openStream.skip(1), "transferTo() != 0"); On 132, it's "skip() != 0". -Brent
Hi Brian,
All previous suggested changes have been made in
99 @Override 100 public int read(byte[] b, int off, int len) throws IOException { 101 Objects.checkFromIndexSize(off, len, b.length); Should not be checked for a null byte buffer b here? -Patrick
Sorry, missed that thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050458.ht... -Patrick Am 09.12.2017 um 22:28 schrieb Patrick Reinhart:
Hi Brian,
All previous suggested changes have been made in
99 @Override 100 public int read(byte[] b, int off, int len) throws IOException { 101 Objects.checkFromIndexSize(off, len, b.length);
Should not be checked for a null byte buffer b here?
-Patrick
On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
All previous suggested changes have been made in
http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
except for the possible change of name for the IS and OS nullStream() methods which awaits consensus.
As long as we’re still at it, given java.util.stream.Stream then InputStream.nullInput() OutputStream.nullOutput() Reader.nullReader() Writer.nullWriter() might not be a bad alternative. Thanks, Brian
On Dec 11, 2017, at 12:52 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
All previous suggested changes have been made in
http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
except for the possible change of name for the IS and OS nullStream() methods which awaits consensus.
As long as we’re still at it, given java.util.stream.Stream then
InputStream.nullInput() OutputStream.nullOutput() Reader.nullReader() Writer.nullWriter()
might not be a bad alternative.
Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are: A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03, B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and C) InputStream.nullInputStream() and OutputStream.nullOutputStream(). Does anyone have any more comments on this point? I would be inclined to choose option C. Thanks, Brian [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.ht... [2] https://bugs.openjdk.java.net/browse/JDK-4358774
On 4 Jan 2018, at 23:42, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Dec 11, 2017, at 12:52 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
All previous suggested changes have been made in
http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
except for the possible change of name for the IS and OS nullStream() methods which awaits consensus.
As long as we’re still at it, given java.util.stream.Stream then
InputStream.nullInput() OutputStream.nullOutput() Reader.nullReader() Writer.nullWriter()
might not be a bad alternative.
Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are:
A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03, B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and C) InputStream.nullInputStream() and OutputStream.nullOutputStream().
Does anyone have any more comments on this point? I would be inclined to choose option C.
After re-reading the email thread, I would choose option C too. It is unambiguous, and the ‘null{classname}’ pattern can be used for Reader/Writer in the future. -Chris
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.ht... [2] https://bugs.openjdk.java.net/browse/JDK-4358774
+1 C) InputStream.nullInputStream() and OutputStream.nullOutputStream(). On 1/5/2018 3:42 AM, Chris Hegarty wrote:
On 4 Jan 2018, at 23:42, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Dec 11, 2017, at 12:52 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
All previous suggested changes have been made in
http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
except for the possible change of name for the IS and OS nullStream() methods which awaits consensus. As long as we’re still at it, given java.util.stream.Stream then
InputStream.nullInput() OutputStream.nullOutput() Reader.nullReader() Writer.nullWriter()
might not be a bad alternative. Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are:
A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03, B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and C) InputStream.nullInputStream() and OutputStream.nullOutputStream().
Does anyone have any more comments on this point? I would be inclined to choose option C. After re-reading the email thread, I would choose option C too. It is unambiguous, and the ‘null{classname}’ pattern can be used for Reader/Writer in the future.
-Chris
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.ht... [2] https://bugs.openjdk.java.net/browse/JDK-4358774
That’s my favorite too… -Patrick
Am 05.01.2018 um 16:31 schrieb Roger Riggs <Roger.Riggs@Oracle.com>:
+1
C) InputStream.nullInputStream() and OutputStream.nullOutputStream().
While there appears now to be agreement on using naming option C below, formal Reviewer approval appears still to be lacking. It would be good if there were at least two approvals forthcoming, if appropriate. http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ Thanks, Brian On Jan 4, 2018, at 3:42 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are:
A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03, B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and C) InputStream.nullInputStream() and OutputStream.nullOutputStream().
Does anyone have any more comments on this point? I would be inclined to choose option C.
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.ht... [2] https://bugs.openjdk.java.net/browse/JDK-4358774
+1 looks fine On 1/11/2018 11:59 AM, Brian Burkhalter wrote:
While there appears now to be agreement on using naming option C below, formal Reviewer approval appears still to be lacking. It would be good if there were at least two approvals forthcoming, if appropriate.
http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
Thanks,
Brian
On Jan 4, 2018, at 3:42 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are:
A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03, B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and C) InputStream.nullInputStream() and OutputStream.nullOutputStream().
Does anyone have any more comments on this point? I would be inclined to choose option C.
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.ht... [2] https://bugs.openjdk.java.net/browse/JDK-4358774
Thanks. The final version is updated in place under webrev.03. Pushing the change is pending CSR-reapproval. Brian On Jan 11, 2018, at 11:16 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
+1 looks fine
On 1/11/2018 11:59 AM, Brian Burkhalter wrote:
While there appears now to be agreement on using naming option C below, formal Reviewer approval appears still to be lacking. It would be good if there were at least two approvals forthcoming, if appropriate.
On 11/01/2018 22:14, Brian Burkhalter wrote:
Thanks. The final version is updated in place under webrev.03. Pushing the change is pending CSR-reapproval.
Looks okay to me. Just a minor nit in the first line of nullInputStream where the javadoc has "contains no bytes". I assume you mean to put "reads" rather than "contains". -Alan
On Jan 12, 2018, at 10:12 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 11/01/2018 22:14, Brian Burkhalter wrote:
Thanks. The final version is updated in place under webrev.03. Pushing the change is pending CSR-reapproval.
Looks okay to me. Just a minor nit in the first line of nullInputStream where the javadoc has "contains no bytes". I assume you mean to put "reads" rather than "contains".
I have not pushed it yet so I will change that in place before doing so. Thanks, Brian
participants (7)
-
Alan Bateman
-
Brent Christian
-
Brian Burkhalter
-
Chris Hegarty
-
Patrick Reinhart
-
Roger Riggs
-
Sergey Bylokhov