RFR 4358774: Add null InputStream and OutputStream

classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Jonathan Gibbons
"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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
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 <[hidden email]> 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".

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Jason Mehrens
In reply to this post by Brian Burkhalter-2
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 <[hidden email]> on behalf of Brian Burkhalter <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
Jason,

On Dec 6, 2017, at 11:54 AM, Jason Mehrens <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Patrick Reinhart
In reply to this post by Brian Burkhalter-2
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 <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Patrick Reinhart

> Am 06.12.2017 um 21:43 schrieb Brian Burkhalter <[hidden email]>:
>
> On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <[hidden email] <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
On Dec 6, 2017, at 12:46 PM, Patrick Reinhart <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Jason Mehrens
In reply to this post by Brian Burkhalter-2
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 <[hidden email]>
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 <[hidden email]<mailto:[hidden email]>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Vitaly Davidovich
On Wed, Dec 6, 2017 at 4:01 PM, Jason Mehrens <[hidden email]>
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 <[hidden email]>
> 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 <[hidden email]<
> mailto:[hidden email]>> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Alan Bateman
In reply to this post by Jason Mehrens


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

roger riggs
In reply to this post by Vitaly Davidovich
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 <[hidden email]>
> 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 <[hidden email]>
>> 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 <[hidden email]<
>> mailto:[hidden email]>> 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
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Vitaly Davidovich
On Thu, Dec 7, 2017 at 9:40 AM Roger Riggs <[hidden email]> 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 <[hidden email]
> >
> > 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 <[hidden email]>
> >> 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 <[hidden email]<
> >> mailto:[hidden email]>> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Pavel Rappo
In reply to this post by Brian Burkhalter-2

> On Dec 6, 2017, at 11:11 AM, Jonathan Gibbons <[hidden email]> 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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Alan Bateman
Updated patch:

http://cr.openjdk.java.net/~bpb/4358774/webrev.01/

On Dec 7, 2017, at 6:08 AM, Alan Bateman <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

roger riggs
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 <[hidden email]
> <mailto:[hidden email]>> 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 <[hidden email]
> <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Brian Burkhalter-2
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 <[hidden email]> 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
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8135248

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Sergey Bylokhov
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 <[hidden email]> 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
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8135248
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
Hi Sergey,

On Dec 7, 2017, at 3:14 PM, Sergey Bylokhov <[hidden email]> wrote:

>> 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?

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
12