Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Peter, On Dec 22, 2017, at 1:21 AM, Peter Levart wrote: > I think this looks good. No more suggestions from my side for this patch. As > Paul notes, it would be interesting to see if using > Unsafe.allocateUninitializedArray() to allocate the final array (and >

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Brian, On 12/21/2017 09:44 PM, Brian Burkhalter wrote: On Dec 21, 2017, at 11:44 AM, Paul Sandoz wrote: I concur that this horse is almost dead from the beatings but since I already hacked up Peter’s suggestion which eliminates intermediate copies I might as well

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 21, 2017, at 11:44 AM, Paul Sandoz wrote: >> I concur that this horse is almost dead from the beatings but since I >> already hacked up Peter’s suggestion which eliminates intermediate copies I >> might as well hang it out there (see below). > > That looks ok to

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

> On 21 Dec 2017, at 11:05, Brian Burkhalter > wrote: > > Hi Paul, > > On Dec 21, 2017, at 10:28 AM, Paul Sandoz > wrote: > >> This looks ok, i think it’s definitely reached it’s complexity budget, and

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Paul, On Dec 21, 2017, at 10:28 AM, Paul Sandoz wrote: > This looks ok, i think it’s definitely reached it’s complexity budget, and > arguably over spent. I concur that this horse is almost dead from the beatings but since I already hacked up Peter’s suggestion

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Peter, On Dec 21, 2017, at 10:26 AM, Brian Burkhalter wrote: > What about the case where read() returns 0, e.g., when reading from a socket, > but subsequent reads return positive values? > >// read to EOF which may read more or less than buffer

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi, This looks ok, i think it’s definitely reached it’s complexity budget, and arguably over spent. — I do have one follow on investigation we discussed off list that is worth measuring. At the end use the Unsafe array allocation with no zeroing, since the resulting array will be fully

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 21, 2017, at 10:23 AM, Peter Levart wrote: >> Or I suppose a single List containing an object containing both the bytes >> and the length would work. One could for example us > > I don't think this would be necessary. All buffers but the last one are fully >

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On 12/21/2017 05:38 PM, Brian Burkhalter wrote: Hi Peter, On Dec 21, 2017, at 2:03 AM, Peter Levart wrote: This is OK as is, but I see another possible improvement to the logic. You decide whether it is worth trying to implement it. Currently the logic reads stream

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Peter, On Dec 21, 2017, at 2:03 AM, Peter Levart wrote: > This is OK as is, but I see another possible improvement to the logic. You > decide whether it is worth trying to implement it. Currently the logic reads > stream into buffers of DEFAULT_BUFFER_SIZE and adds

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 21, 2017, at 3:05 AM, Alan Bateman wrote: > On 20/12/2017 22:30, Brian Burkhalter wrote: >> : >> http://cr.openjdk.java.net/~bpb/8193832/webrev.03/ >> >> The patch is updated to: >> >> * use Peter’s approach to avoid allocating an ArrayList when length <= >>

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On 20/12/2017 22:30, Brian Burkhalter wrote: : http://cr.openjdk.java.net/~bpb/8193832/webrev.03/ The patch is updated to: * use Peter’s approach to avoid allocating an ArrayList when length <= DEFAULT_BUFFER_SIZE; * use the default ArrayList constructor instead of that with a specific

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Brian, On 12/20/2017 11:30 PM, Brian Burkhalter wrote: On Dec 20, 2017, at 11:52 AM, Paul Sandoz wrote: I was a little lassiaz-faire given that 8K bytes were anyway being allocated upfront. Peter’s changes look good. Brian, i would double check the tests to make

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Brian, Brian Burkhalter je 20. 12. 2017 ob 22:54 napisal: Hi Peter, On Dec 20, 2017, at 3:45 AM, Peter Levart > wrote:     if (result == null) { result = copy;     } else { bufs = new ArrayList<>(8); // <— ?

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 20, 2017, at 11:52 AM, Paul Sandoz wrote: > I was a little lassiaz-faire given that 8K bytes were anyway being allocated > upfront. Peter’s changes look good. > > Brian, i would double check the tests to make sure the various code paths are > tested.

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Peter, On Dec 20, 2017, at 3:45 AM, Peter Levart wrote: > if (result == null) { > result = copy; > } else { > bufs = new ArrayList<>(8); // <— ? > bufs.add(result); >

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

> On 20 Dec 2017, at 11:04, John Rose wrote: > > On Dec 20, 2017, at 3:45 AM, Peter Levart > wrote: >> >> allocation of ArrayList could be avoided. Imagine a use case where lots of >> small files are read into

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 20, 2017, at 3:45 AM, Peter Levart wrote: > > allocation of ArrayList could be avoided. Imagine a use case where lots of > small files are read into byte[] arrays +1 I was going to write a similar suggestion; thanks for sending it out. These sorts of things

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Peter, On Dec 20, 2017, at 8:04 AM, Peter Levart wrote: >>> found another improvement. If you are reading from files, there's no >>> difference. But if you read from say socket(s), there may be short reads >>> (read() returning 0). >> InputStreams are blocking so

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On 12/20/2017 03:09 PM, Alan Bateman wrote: On 20/12/2017 12:40, Peter Levart wrote: Hi Brian, I found another improvement. If you are reading from files, there's no difference. But if you read from say socket(s), there may be short reads (read() returning 0). InputStreams are blocking

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On 19/12/17 23:22, Brian Burkhalter wrote: .. Updated version here: http://cr.openjdk.java.net/~bpb/8193832/webrev.02/ Looks good to me. This is a nice improvement on the original implementation that I added in 9. Thanks. -Chris.

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On 20/12/2017 12:40, Peter Levart wrote: Hi Brian, I found another improvement. If you are reading from files, there's no difference. But if you read from say socket(s), there may be short reads (read() returning 0). InputStreams are blocking so if someone creates an InputStream over a

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Brian, I found another improvement. If you are reading from files, there's no difference. But if you read from say socket(s), there may be short reads (read() returning 0). Your current code bails out from inner loop when this happens and consequently does not fill the buf up to the top

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Brian, There's also a variation of copy-ing fragment possible, that replaces copying with allocation:     byte[] copy;     if (nread == DEFAULT_BUFFER_SIZE) {     copy = buf;     if (n >= 0) {     buf = new

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi Brian, On 12/20/2017 12:22 AM, Brian Burkhalter wrote: On Dec 19, 2017, at 2:36 PM, Brian Burkhalter wrote: You can also simplify the “for(;;) + break" into a do while loop: do { int nread = 0; ... } while (n > 0); Good suggestion but I think that this

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 19, 2017, at 2:36 PM, Brian Burkhalter wrote: >> You can also simplify the “for(;;) + break" into a do while loop: >> >> do { >> int nread = 0; >> ... >> } while (n > 0); > > Good suggestion but I think that this needs to be "while (n >= 0)." Updated

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 19, 2017, at 2:28 PM, Paul Sandoz wrote: >> Done. Updated: http://cr.openjdk.java.net/~bpb/8193832/webrev.01/ >> > > You can also simplify the “for(;;) + break" into a do while loop: > > do { > int nread = 0; > ... > } while (n > 0); Good suggestion but I

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

> On 19 Dec 2017, at 13:43, Brian Burkhalter > wrote: > > On Dec 19, 2017, at 12:52 PM, Paul Sandoz > wrote: > >> For the case of reading 2^N bytes i believe you can avoid doing a last copy >> by checking

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

On Dec 19, 2017, at 12:52 PM, Paul Sandoz wrote: > For the case of reading 2^N bytes i believe you can avoid doing a last copy > by checking if “n < 0" within the “nread > 0” block when “nread == > DEAFULT_BUFFER_SIZE”. That might close the perf gap for smaller cases.

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

- Mail original - > De: "Paul Sandoz" <paul.san...@oracle.com> > À: "Brian Burkhalter" <brian.burkhal...@oracle.com> > Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> > Envoyé: Mardi 19 Décembre 2017 21:52:03 > Objet: R

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

Hi, For the case of reading 2^N bytes i believe you can avoid doing a last copy by checking if “n < 0" within the “nread > 0” block when “nread == DEAFULT_BUFFER_SIZE”. That might close the perf gap for smaller cases. You can also move "nread = 0” to the same block e.g.: var copy = (n < 0

RFR 8193832: Performance of InputStream.readAllBytes() could be improved

https://bugs.openjdk.java.net/browse/JDK-8193832 http://cr.openjdk.java.net/~bpb/8193832/webrev.00/ The implementation of InputStream.readAllBytes() can be modified to be in general faster and to require less memory. The current implementation starts with an internal buffer of size 8192 bytes