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

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

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

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

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

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?

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?