RFR 4358774: Add null InputStream and OutputStream
https://bugs.openjdk.java.net/browse/JDK-4358774 http://cr.openjdk.java.net/~bpb/4358774/webrev.00/ Add nullStream() method to each of InputStream and OutputStream. Thanks, Brian
"null" is a significant term in the Java ecosystem, and the relationship here, to /dev/null or NUL seems somewhat tenuous. Have any other names been considered? At least for the InputStream, calling it an "empty stream" seems more intuitive than a "null stream". -- Jon On 12/6/17 11:00 AM, Brian Burkhalter wrote:
https://bugs.openjdk.java.net/browse/JDK-4358774 http://cr.openjdk.java.net/~bpb/4358774/webrev.00/
Add nullStream() method to each of InputStream and OutputStream.
Thanks,
Brian
The name “emptyStream()” was considered for InputStream and “discardingStream()” for OutputStream. It was thought that “null” or “empty” would be more likely to be found by developers due to familiarity. FWIW there is precedent in third party libraries for the “null” names. Brian On Dec 6, 2017, at 11:11 AM, Jonathan Gibbons <jonathan.gibbons@oracle.com> wrote:
"null" is a significant term in the Java ecosystem, and the relationship here, to /dev/null or NUL seems somewhat tenuous.
Have any other names been considered? At least for the InputStream, calling it an "empty stream" seems more intuitive than a "null stream".
On Dec 6, 2017, at 11:11 AM, Jonathan Gibbons <jonathan.gibbons@oracle.com> wrote:
"null" is a significant term in the Java ecosystem, and the relationship here, to /dev/null or NUL seems somewhat tenuous.
Have any other names been considered? At least for the InputStream, calling it an "empty stream" seems more intuitive than a "null stream".
Jon, I think there's an established name for this kind of objects: https://en.wikipedia.org/wiki/Null_object_pattern
On 6 Dec 2017, at 22:28, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
The name “emptyStream()” was considered for InputStream and “discardingStream()” for OutputStream. It was thought that “null” or “empty” would be more likely to be found by developers due to familiarity. FWIW there is precedent in third party libraries for the “null” names.
Brian
+1
Brian, For nullInputStream would it make any sense to use the ByteArrayInputStream with a (private static) empty byte array? Maybe 'return new ByteArrayInputStream("".getBytes());'? One side effect is that mark support returns true. Does it make sense to follow the advice in https://bugs.openjdk.java.net/browse/JDK-6533165 with regards to streams being closed? Thanks, Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Brian Burkhalter <brian.burkhalter@oracle.com> Sent: Wednesday, December 6, 2017 1:00 PM To: core-libs-dev Subject: RFR 4358774: Add null InputStream and OutputStream https://bugs.openjdk.java.net/browse/JDK-4358774 http://cr.openjdk.java.net/~bpb/4358774/webrev.00/ Add nullStream() method to each of InputStream and OutputStream. Thanks, Brian
Jason, On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jason_mehrens@hotmail.com> wrote:
For nullInputStream would it make any sense to use the ByteArrayInputStream with a (private static) empty byte array? Maybe 'return new ByteArrayInputStream("".getBytes());'? One side effect is that mark support returns true.
As we are trying to retain the behavior of closed streams throwing IOExceptions I don’t think that BAIS would work here.
Does it make sense to follow the advice inhttps://bugs.openjdk.java.net/browse/JDK-6533165 with regards to streams being closed?
I don’t know exactly what you intend here. In the linked issue there is information to impart, namely the index and the size. Here there is only the indication that the stream is closed. It’s not clear to me what could be done here. Thanks, Brian
Brian, My understand is JDK-6533165 is moving the bytes that are rarely invoked to a cold method. Therefore: ==== if (closed) { throw new IOException("Stream closed"); } ==becomes=== if (closed) { throw sc(); } private static IOException sc() { return new IOException("Stream closed"); } ================================ Since there is no string concatenation in the IOE message there are only a few bytes that can be shaved off. I didn't benchmark it either case but I would assume it would matter for the nullOutputStream since the write methods could be invoked multiple times. Jason ________________________________________ From: Brian Burkhalter <brian.burkhalter@oracle.com> Sent: Wednesday, December 6, 2017 2:05 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR 4358774: Add null InputStream and OutputStream Jason, On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jason_mehrens@hotmail.com<mailto:jason_mehrens@hotmail.com>> wrote: For nullInputStream would it make any sense to use the ByteArrayInputStream with a (private static) empty byte array? Maybe 'return new ByteArrayInputStream("".getBytes());'? One side effect is that mark support returns true. As we are trying to retain the behavior of closed streams throwing IOExceptions I don’t think that BAIS would work here. Does it make sense to follow the advice inhttps://bugs.openjdk.java.net/browse/JDK-6533165 with regards to streams being closed? I don’t know exactly what you intend here. In the linked issue there is information to impart, namely the index and the size. Here there is only the indication that the stream is closed. It’s not clear to me what could be done here. Thanks, Brian
On Wed, Dec 6, 2017 at 4:01 PM, Jason Mehrens <jason_mehrens@hotmail.com> wrote:
Brian,
My understand is JDK-6533165 is moving the bytes that are rarely invoked to a cold method.
Therefore: ==== if (closed) { throw new IOException("Stream closed"); } ==becomes=== if (closed) { throw sc(); }
private static IOException sc() { return new IOException("Stream closed"); } ================================
Since there is no string concatenation in the IOE message there are only a few bytes that can be shaved off. I didn't benchmark it either case but I would assume it would matter for the nullOutputStream since the write methods could be invoked multiple times.
From a performance angle, I'd be more concerned with the calls to Objects.xyz() methods there. Unless something has changed in the JIT recently, those are susceptible to profile pollution and can cause missed optimizations. I'd inline those methods manually to give these methods their own profiles.
Jason ________________________________________ From: Brian Burkhalter <brian.burkhalter@oracle.com> Sent: Wednesday, December 6, 2017 2:05 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR 4358774: Add null InputStream and OutputStream
Jason,
On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jason_mehrens@hotmail.com< mailto:jason_mehrens@hotmail.com>> wrote:
For nullInputStream would it make any sense to use the ByteArrayInputStream with a (private static) empty byte array? Maybe 'return new ByteArrayInputStream("".getBytes());'? One side effect is that mark support returns true.
As we are trying to retain the behavior of closed streams throwing IOExceptions I don’t think that BAIS would work here.
Does it make sense to follow the advice inhttps://bugs.openjdk.java. net/browse/JDK-6533165 with regards to streams being closed?
I don’t know exactly what you intend here. In the linked issue there is information to impart, namely the index and the size. Here there is only the indication that the stream is closed. It’s not clear to me what could be done here.
Thanks,
Brian
Hmm... Seems like a lot of fine tuning for a function that has a low profile and no performance data... $.02, Roger On 12/6/2017 10:03 PM, Vitaly Davidovich wrote:
On Wed, Dec 6, 2017 at 4:01 PM, Jason Mehrens <jason_mehrens@hotmail.com> wrote:
Brian,
My understand is JDK-6533165 is moving the bytes that are rarely invoked to a cold method.
Therefore: ==== if (closed) { throw new IOException("Stream closed"); } ==becomes=== if (closed) { throw sc(); }
private static IOException sc() { return new IOException("Stream closed"); } ================================
Since there is no string concatenation in the IOE message there are only a few bytes that can be shaved off. I didn't benchmark it either case but I would assume it would matter for the nullOutputStream since the write methods could be invoked multiple times.
From a performance angle, I'd be more concerned with the calls to Objects.xyz() methods there. Unless something has changed in the JIT recently, those are susceptible to profile pollution and can cause missed optimizations. I'd inline those methods manually to give these methods their own profiles.
Jason ________________________________________ From: Brian Burkhalter <brian.burkhalter@oracle.com> Sent: Wednesday, December 6, 2017 2:05 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR 4358774: Add null InputStream and OutputStream
Jason,
On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jason_mehrens@hotmail.com< mailto:jason_mehrens@hotmail.com>> wrote:
For nullInputStream would it make any sense to use the ByteArrayInputStream with a (private static) empty byte array? Maybe 'return new ByteArrayInputStream("".getBytes());'? One side effect is that mark support returns true.
As we are trying to retain the behavior of closed streams throwing IOExceptions I don’t think that BAIS would work here.
Does it make sense to follow the advice inhttps://bugs.openjdk.java. net/browse/JDK-6533165 with regards to streams being closed?
I don’t know exactly what you intend here. In the linked issue there is information to impart, namely the index and the size. Here there is only the indication that the stream is closed. It’s not clear to me what could be done here.
Thanks,
Brian
On Thu, Dec 7, 2017 at 9:40 AM Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hmm...
Seems like a lot of fine tuning for a function that has a low profile and no performance data...
These are known issues in the current JIT optimizer. We all wish we didn’t have to do this stuff. If we want the best chance of these noop impls to actually be close to noop then it warrants consideration. If that’s not a concern then sure, nothing to discuss. That’s all :). I also wouldn’t say “it’s a lot of fine tuning” - these are simple mechanical things.
$.02, Roger
On 12/6/2017 10:03 PM, Vitaly Davidovich wrote:
On Wed, Dec 6, 2017 at 4:01 PM, Jason Mehrens <jason_mehrens@hotmail.com
wrote:
Brian,
My understand is JDK-6533165 is moving the bytes that are rarely invoked to a cold method.
Therefore: ==== if (closed) { throw new IOException("Stream closed"); } ==becomes=== if (closed) { throw sc(); }
private static IOException sc() { return new IOException("Stream closed"); } ================================
Since there is no string concatenation in the IOE message there are only a few bytes that can be shaved off. I didn't benchmark it either case but I would assume it would matter for the nullOutputStream since the write methods could be invoked multiple times.
From a performance angle, I'd be more concerned with the calls to Objects.xyz() methods there. Unless something has changed in the JIT recently, those are susceptible to profile pollution and can cause missed optimizations. I'd inline those methods manually to give these methods their own profiles.
Jason ________________________________________ From: Brian Burkhalter <brian.burkhalter@oracle.com> Sent: Wednesday, December 6, 2017 2:05 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR 4358774: Add null InputStream and OutputStream
Jason,
On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jason_mehrens@hotmail.com< mailto:jason_mehrens@hotmail.com>> wrote:
For nullInputStream would it make any sense to use the ByteArrayInputStream with a (private static) empty byte array? Maybe 'return new ByteArrayInputStream("".getBytes());'? One side effect is that mark support returns true.
As we are trying to retain the behavior of closed streams throwing IOExceptions I don’t think that BAIS would work here.
Does it make sense to follow the advice inhttps://bugs.openjdk.java. net/browse/JDK-6533165 with regards to streams being closed?
I don’t know exactly what you intend here. In the linked issue there is information to impart, namely the index and the size. Here there is only the indication that the stream is closed. It’s not clear to me what could be done here.
Thanks,
Brian
--
Sent from my phone
On 06/12/2017 21:01, Jason Mehrens wrote:
Brian,
My understand is JDK-6533165 is moving the bytes that are rarely invoked to a cold method.
Therefore: ==== if (closed) { throw new IOException("Stream closed"); } ==becomes=== if (closed) { throw sc(); }
private static IOException sc() { return new IOException("Stream closed"); } ================================
If nothing else, a private ensureOpen method would make it easier to read so that the "if (closed) throw ..." isn't needed in every method. -Alan
Updated patch: http://cr.openjdk.java.net/~bpb/4358774/webrev.01/ On Dec 7, 2017, at 6:08 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
If nothing else, a private ensureOpen method would make it easier to read so that the "if (closed) throw ..." isn't needed in every method.
Done. On Dec 6, 2017, at 7:03 PM, Vitaly Davidovich <vitalyd@gmail.com> wrote:
From a performance angle, I'd be more concerned with the calls to Objects.xyz() methods there. Unless something has changed in the JIT recently, those are susceptible to profile pollution and can cause missed optimizations. I'd inline those methods manually to give these methods their own profiles.
Done. Thanks, Brian
Hi Brian, 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 [1] https://bugs.openjdk.java.net/browse/JDK-8135248 On 12/7/2017 2:27 PM, Brian Burkhalter wrote:
Updated patch:
http://cr.openjdk.java.net/~bpb/4358774/webrev.01/ <http://cr.openjdk.java.net/%7Ebpb/4358774/webrev.01/>
On Dec 7, 2017, at 6:08 AM, Alan Bateman <Alan.Bateman@oracle.com <mailto:Alan.Bateman@oracle.com>> wrote:
If nothing else, a private ensureOpen method would make it easier to read so that the "if (closed) throw ..." isn't needed in every method.
Done.
On Dec 6, 2017, at 7:03 PM, Vitaly Davidovich <vitalyd@gmail.com <mailto:vitalyd@gmail.com>> wrote:
From a performance angle, I'd be more concerned with the calls to Objects.xyz() methods there. Unless something has changed in the JIT recently, those are susceptible to profile pollution and can cause missed optimizations. I'd inline those methods manually to give these methods their own profiles.
Done.
Thanks,
Brian
Sounds great, perfect to remove some more own code… Is there also a issue for the same kind of methods for Reader and Writer? -Patrick
Am 06.12.2017 um 20:00 schrieb Brian Burkhalter <brian.burkhalter@oracle.com>:
https://bugs.openjdk.java.net/browse/JDK-4358774 http://cr.openjdk.java.net/~bpb/4358774/webrev.00/
Add nullStream() method to each of InputStream and OutputStream.
Thanks,
Brian
On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <patrick@reini.net> wrote:
Sounds great, perfect to remove some more own code…
Yes I still need to look through the JDK source base for some code to replace with this, assuming this is approved.
Is there also a issue for the same kind of methods for Reader and Writer?
Nothing officially on file yet but it’s “on the list” somewhere. Brian
Am 06.12.2017 um 21:43 schrieb Brian Burkhalter <brian.burkhalter@oracle.com>:
On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <patrick@reini.net <mailto:patrick@reini.net>> wrote:
Sounds great, perfect to remove some more own code…
Yes I still need to look through the JDK source base for some code to replace with this, assuming this is approved.
You got min - even I’m not eligible to give you an official one :-)
Is there also a issue for the same kind of methods for Reader and Writer?
Nothing officially on file yet but it’s “on the list” somewhere.
Would be a something that I could work on as soon you got the approval… -Patrick
On Dec 6, 2017, at 12:46 PM, Patrick Reinhart <patrick@reini.net> wrote:
Yes I still need to look through the JDK source base for some code to replace with this, assuming this is approved.
You got min - even I’m not eligible to give you an official one :-)
Thanks.
Is there also a issue for the same kind of methods for Reader and Writer?
Nothing officially on file yet but it’s “on the list” somewhere.
Would be a something that I could work on as soon you got the approval…
Sure. Thanks, Brian
Patrick’s comment made us think again about the naming here as “nullStream()” would not fit for eventual equivalent methods on Reader and Writer. It might be better to go with something like InputStream InputStream.nullSource(); OutputStream.nullSink(); and later Reader.nullSource(); Writer.nullSink(); Another alternative would be simply to reflect the class names in the methods: InputStream InputStream.nullInputStream(); OutputStream.nullOutputStream(); and later Reader.nullReader(); Writer.nullWriter(); Comments? Thanks, Brian On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <patrick@reini.net> wrote:
Is there also a issue for the same kind of methods for Reader and Writer?
On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Patrick’s comment made us think again about the naming here as “nullStream()” would not fit for eventual equivalent methods on Reader and Writer. It might be better to go with something like
InputStream InputStream.nullSource(); OutputStream.nullSink();
and later
Reader.nullSource(); Writer.nullSink();
Another alternative would be simply to reflect the class names in the methods:
InputStream InputStream.nullInputStream(); OutputStream.nullOutputStream();
and later
Reader.nullReader(); Writer.nullWriter();
I for one prefer this alternative; it's very clear and unambiguous. -- - DML
I like the nullInputStream() nullOutputStream() as I would first search for those names, also nullReader() / nullWriter() seem to fit more than Sink/Source or Stream -Patrick
Am 08.12.2017 um 19:38 schrieb Brian Burkhalter <brian.burkhalter@oracle.com>:
Patrick’s comment made us think again about the naming here as “nullStream()” would not fit for eventual equivalent methods on Reader and Writer. It might be better to go with something like
InputStream InputStream.nullSource(); OutputStream.nullSink();
and later
Reader.nullSource(); Writer.nullSink();
Another alternative would be simply to reflect the class names in the methods:
InputStream InputStream.nullInputStream(); OutputStream.nullOutputStream();
and later
Reader.nullReader(); Writer.nullWriter();
Comments?
Thanks,
Brian
On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <patrick@reini.net> wrote:
Is there also a issue for the same kind of methods for Reader and Writer?
participants (9)
-
Alan Bateman
-
Brian Burkhalter
-
David Lloyd
-
Jason Mehrens
-
Jonathan Gibbons
-
Patrick Reinhart
-
Pavel Rappo
-
Roger Riggs
-
Vitaly Davidovich