Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Jan 12, 2018, at 10:12 AM, Alan Bateman  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

Re: RFR 4358774: Add null InputStream and OutputStream

[Alan Bateman]

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

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  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.
>> 
>> http://cr.openjdk.java.net/~bpb/4358774/webrev.03/

Re: RFR 4358774: Add null InputStream and OutputStream

[Roger Riggs]

+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  
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.html
[2] https://bugs.openjdk.java.net/browse/JDK-4358774

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

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  
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.html
> [2] https://bugs.openjdk.java.net/browse/JDK-4358774

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

That’s my favorite too…

-Patrick

> Am 05.01.2018 um 16:31 schrieb Roger Riggs :
> 
> +1
> 
> C) InputStream.nullInputStream() and OutputStream.nullOutputStream().

Re: RFR 4358774: Add null InputStream and OutputStream

[Roger Riggs]

+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  wrote:


On Dec 11, 2017, at 12:52 PM, Brian Burkhalter  
wrote:


On Dec 8, 2017, at 3:12 PM, Brian Burkhalter  
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.html
[2] https://bugs.openjdk.java.net/browse/JDK-4358774

Re: RFR 4358774: Add null InputStream and OutputStream

[Chris Hegarty]

> On 4 Jan 2018, at 23:42, Brian Burkhalter  wrote:
> 
>> On Dec 11, 2017, at 12:52 PM, Brian Burkhalter  
>> wrote:
>> 
>>> On Dec 8, 2017, at 3:12 PM, Brian Burkhalter  
>>> 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.html
> [2] https://bugs.openjdk.java.net/browse/JDK-4358774

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 11, 2017, at 12:52 PM, Brian Burkhalter  
wrote:

> On Dec 8, 2017, at 3:12 PM, Brian Burkhalter  
> 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.html
[2] https://bugs.openjdk.java.net/browse/JDK-4358774

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 8, 2017, at 3:12 PM, Brian Burkhalter  
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

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

Sorry, missed that thread:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050458.html

-Patrick

Am 09.12.2017 um 22:28 schrieb Patrick Reinhart:
> Hi Brian,
>
>> All previous suggested changes have been made in
>>
>> http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
>>
>   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

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

Hi Brian,

> All previous suggested changes have been made in
> 
> http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
> 

  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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 8, 2017, at 5:06 PM, Sergey Bylokhov  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: " If len is zero, then 
> no bytes are read and 0 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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

Hi Brent,

On Dec 8, 2017, at 4:50 PM, Brent Christian  wrote:

> I just noticed a couple small things in the test:
> 
> test/jdk/java/io/InputStream/NullInputStream.java
> 
> […]
> 
> Line 113 should read "... != 0", yes?
> 
> […]
> 
> On 132, it's "skip() != 0".

You are correct on both accounts. I shuffled that test completely around today 
converting it to TestNG and obviously made some typos. Webrev updated in place 
http://cr.openjdk.java.net/~bpb/4358774/webrev.03/.

Thanks,

Brian

Re: RFR 4358774: Add null InputStream and OutputStream

[Sergey Bylokhov]

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: " If len is zero, 
then no bytes are read and 0 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.

Re: RFR 4358774: Add null InputStream and OutputStream

[Brent Christian]

Hi, Brian

On 12/8/17 3:12 PM, Brian Burkhalter wrote:


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



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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

Hi Sergey,

On Dec 8, 2017, at 4:34 PM, Sergey Bylokhov  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/classes/java/io/InputStream.java#l166

Re: RFR 4358774: Add null InputStream and OutputStream

[Sergey Bylokhov]

Hi, Brian.
On 08/12/2017 15:12, Brian Burkhalter wrote:

All previous suggested changes have been made in

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


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.

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 8, 2017, at 8:29 AM, Brian Burkhalter  
wrote:

> On Dec 8, 2017, at 4:39 AM, Alan Bateman  wrote:
> 
>>> 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.
> 
> 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

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

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 :
> 
> 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  wrote:
> 
>> Is there also a issue for the same kind of methods for Reader and Writer?
> 

Re: RFR 4358774: Add null InputStream and OutputStream

[David Lloyd]

On Fri, Dec 8, 2017 at 1:03 PM, Brian Burkhalter
 wrote:
> On Dec 8, 2017, at 10:52 AM, David Lloyd  wrote:
>
>> On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
>>  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.
> The second one?

Yes, sorry.

-- 
- DML

Re: RFR 4358774: Add null InputStream and OutputStream

[Jonathan Bluett-Duncan]

Like David, I prefer the `null{$ClassName}` alternative to
`null{Source|Sink}`. (I assume from his wording that he prefers
`null{$ClassName}`.)

I prefer `null{$ClassName}` not only because it's less ambiguous, but also
because Guava has existing types `{Byte|Char}Source` and `{Byte|Char}Sink`,
so I'd find it a bit confusing to see `nullSource()` at first (especially
if it were statically-imported) as I would initially expect it to have a
return type of `*Source`.

Cheers,
Jonathan

On 8 December 2017 at 19:03, Brian Burkhalter 
wrote:

> On Dec 8, 2017, at 10:52 AM, David Lloyd  wrote:
>
> > On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
> >  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.
> The second one?
>
> Thanks,
>
> Brian
>
>

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 8, 2017, at 10:52 AM, David Lloyd  wrote:

> On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
>  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.
The second one?

Thanks,

Brian

Re: RFR 4358774: Add null InputStream and OutputStream

[David Lloyd]

On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
 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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

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  wrote:

> Is there also a issue for the same kind of methods for Reader and Writer?

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 8, 2017, at 4:39 AM, Alan Bateman  wrote:

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

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Alan Bateman]

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

Hi Sergey,

On Dec 7, 2017, at 3:14 PM, Sergey Bylokhov  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

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

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

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/ 



On Dec 7, 2017, at 6:08 AM, Alan Bateman > 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 > 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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

Updated patch:

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

On Dec 7, 2017, at 6:08 AM, Alan Bateman  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  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

Re: RFR 4358774: Add null InputStream and OutputStream

[Pavel Rappo]

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Vitaly Davidovich]

On Thu, Dec 7, 2017 at 9:40 AM Roger Riggs <roger.ri...@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_mehr...@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.burkhal...@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_mehr...@hotmail.com<
> >> mailto:jason_mehr...@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

Re: RFR 4358774: Add null InputStream and OutputStream

[Roger Riggs]

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_mehr...@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.burkhal...@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_mehr...@hotmail.com<
mailto:jason_mehr...@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

Re: RFR 4358774: Add null InputStream and OutputStream

[Alan Bateman]

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Vitaly Davidovich]

On Wed, Dec 6, 2017 at 4:01 PM, Jason Mehrens <jason_mehr...@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.burkhal...@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_mehr...@hotmail.com<
> mailto:jason_mehr...@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
>

Re: RFR 4358774: Add null InputStream and OutputStream

[Jason Mehrens]

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.burkhal...@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_mehr...@hotmail.com<mailto:jason_mehr...@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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 6, 2017, at 12:46 PM, Patrick Reinhart  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

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

On Dec 6, 2017, at 12:36 PM, Patrick Reinhart  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

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

Jason,

On Dec 6, 2017, at 11:54 AM, Jason Mehrens  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

Re: RFR 4358774: Add null InputStream and OutputStream

[Jason Mehrens]

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-boun...@openjdk.java.net> on behalf of Brian 
Burkhalter <brian.burkhal...@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

Re: RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

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  
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".

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

RFR 4358774: Add null InputStream and OutputStream

[Brian Burkhalter]

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