Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v4]

[Brian Burkhalter]

> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
> improve its performance when `skip(long)` skips fewer than the requested 
> number of bytes. In the current implementation, `skip(long)` is invoked once 
> and, if not enough bytes have been skipped, then `read()` is invoked for each 
> of the remaining bytes to be skipped. The proposed implementation instead 
> repeatedly invokes `skip(long)` until the requested number of bytes has been 
> skipped, or an error condition is encountered. For cases where `skip(long)` 
> skips fewer bytes than the number requested, the new version was measured to 
> be up to more than one thousand times faster than the old version. When 
> `skip(long)` actually skips the requested number of bytes, the performance 
> difference is insignificant.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8246739: InputStream.skipNBytes could be implemented more efficiently

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1329/files
  - new: https://git.openjdk.java.net/jdk/pull/1329/files/60fe0953..31678bfa

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1329=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1329=02-03

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1329.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1329/head:pull/1329

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v3]

[Brian Burkhalter]

> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
> improve its performance when `skip(long)` skips fewer than the requested 
> number of bytes. In the current implementation, `skip(long)` is invoked once 
> and, if not enough bytes have been skipped, then `read()` is invoked for each 
> of the remaining bytes to be skipped. The proposed implementation instead 
> repeatedly invokes `skip(long)` until the requested number of bytes has been 
> skipped, or an error condition is encountered. For cases where `skip(long)` 
> skips fewer bytes than the number requested, the new version was measured to 
> be up to more than one thousand times faster than the old version. When 
> `skip(long)` actually skips the requested number of bytes, the performance 
> difference is insignificant.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8246739: InputStream.skipNBytes could be implemented more efficiently

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1329/files
  - new: https://git.openjdk.java.net/jdk/pull/1329/files/f59d7a35..60fe0953

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1329=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1329=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1329.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1329/head:pull/1329

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[Brian Burkhalter]

On Fri, 20 Nov 2020 11:43:11 GMT, sergus13 
 wrote:

>> Brian Burkhalter has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>
> src/java.base/share/classes/java/io/InputStream.java line 605:
> 
>> 603: } else if (ns == 0) { // no bytes skipped
>> 604: // read one byte to check for EOS
>> 605: if (read() < 0) {
> 
> Wouldn't it be better to compare return value with -1 (as it was before)?
> Since the documentation of read method says:
> _Returns:
> the next byte of data, or **-1 if the end of the stream is reached**._

Indeed that would match the letter of the specification of `read()` but I don't 
think it would make any practical difference.

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[sergus13]

On Thu, 19 Nov 2020 20:20:17 GMT, Brian Burkhalter  wrote:

>> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
>> improve its performance when `skip(long)` skips fewer than the requested 
>> number of bytes. In the current implementation, `skip(long)` is invoked once 
>> and, if not enough bytes have been skipped, then `read()` is invoked for 
>> each of the remaining bytes to be skipped. The proposed implementation 
>> instead repeatedly invokes `skip(long)` until the requested number of bytes 
>> has been skipped, or an error condition is encountered. For cases where 
>> `skip(long)` skips fewer bytes than the number requested, the new version 
>> was measured to be up to more than one thousand times faster than the old 
>> version. When `skip(long)` actually skips the requested number of bytes, the 
>> performance difference is insignificant.
>
> Brian Burkhalter has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently

src/java.base/share/classes/java/io/InputStream.java line 605:

> 603: } else if (ns == 0) { // no bytes skipped
> 604: // read one byte to check for EOS
> 605: if (read() < 0) {

Wouldn't it be better to compare return value with -1 (as it was before)?
Since the documentation of read method says:
_Returns:
the next byte of data, or **-1 if the end of the stream is reached**._

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[sergus13]

On Thu, 19 Nov 2020 20:20:17 GMT, Brian Burkhalter  wrote:

>> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
>> improve its performance when `skip(long)` skips fewer than the requested 
>> number of bytes. In the current implementation, `skip(long)` is invoked once 
>> and, if not enough bytes have been skipped, then `read()` is invoked for 
>> each of the remaining bytes to be skipped. The proposed implementation 
>> instead repeatedly invokes `skip(long)` until the requested number of bytes 
>> has been skipped, or an error condition is encountered. For cases where 
>> `skip(long)` skips fewer bytes than the number requested, the new version 
>> was measured to be up to more than one thousand times faster than the old 
>> version. When `skip(long)` actually skips the requested number of bytes, the 
>> performance difference is insignificant.
>
> Brian Burkhalter has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently

src/java.base/share/classes/java/io/InputStream.java line 607:

> 605: if (read() < 0) {
> 606: throw new EOFException();
> 607: }

Shouldn't we decrement "n" in case "read" returns non-negative value?

if (read() < 0) {
throw new EOFException();
} else {
n--;
}
`

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[sergus13]

On Fri, 20 Nov 2020 09:56:16 GMT, sergus13 
 wrote:

>> Brian Burkhalter has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>
> src/java.base/share/classes/java/io/InputStream.java line 607:
> 
>> 605: if (read() < 0) {
>> 606: throw new EOFException();
>> 607: }
> 
> Shouldn't we decrement "n" in case "read" returns non-negative value?
> 
> if (read() < 0) {
> throw new EOFException();
> } else {
> n--;
> }
> `

One more notice here. Wouldn't it be better to compare value returned by read 
method with -1 (as it was before)?
Since documentation of read method says:
_Returns:
the next byte of data, or -1 if the end of the stream is reached._

`
if (read() != -1) {
throw new EOFException();
} else {
n--;
}
`

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[Naoto Sato]

On Thu, 19 Nov 2020 21:41:00 GMT, Naoto Sato  wrote:

>> Brian Burkhalter has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>
> Marked as reviewed by naoto (Reviewer).

> CSR filed [1]
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8256698

Looks good. Reviewed.

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[Naoto Sato]

On Thu, 19 Nov 2020 20:20:17 GMT, Brian Burkhalter  wrote:

>> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
>> improve its performance when `skip(long)` skips fewer than the requested 
>> number of bytes. In the current implementation, `skip(long)` is invoked once 
>> and, if not enough bytes have been skipped, then `read()` is invoked for 
>> each of the remaining bytes to be skipped. The proposed implementation 
>> instead repeatedly invokes `skip(long)` until the requested number of bytes 
>> has been skipped, or an error condition is encountered. For cases where 
>> `skip(long)` skips fewer bytes than the number requested, the new version 
>> was measured to be up to more than one thousand times faster than the old 
>> version. When `skip(long)` actually skips the requested number of bytes, the 
>> performance difference is insignificant.
>
> Brian Burkhalter has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently

Marked as reviewed by naoto (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[Lance Andersen]

On Thu, 19 Nov 2020 20:20:17 GMT, Brian Burkhalter  wrote:

>> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
>> improve its performance when `skip(long)` skips fewer than the requested 
>> number of bytes. In the current implementation, `skip(long)` is invoked once 
>> and, if not enough bytes have been skipped, then `read()` is invoked for 
>> each of the remaining bytes to be skipped. The proposed implementation 
>> instead repeatedly invokes `skip(long)` until the requested number of bytes 
>> has been skipped, or an error condition is encountered. For cases where 
>> `skip(long)` skips fewer bytes than the number requested, the new version 
>> was measured to be up to more than one thousand times faster than the old 
>> version. When `skip(long)` actually skips the requested number of bytes, the 
>> performance difference is insignificant.
>
> Brian Burkhalter has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently

Marked as reviewed by lancea (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently

[Brian Burkhalter]

> On Nov 19, 2020, at 12:04 PM, Brian Burkhalter  wrote:
> 
> On Thu, 19 Nov 2020 19:58:16 GMT, Naoto Sato  > wrote:
> 
>> The code looks good. Should we also change the @implSpec, as it specifically 
>> describes how it works, i.e., calling no-arg read(), but the new impl calls 
>> read() with buffer.
> 
> Good catch. Yes, the `@implSpec` should be updated. I was too focused on the 
> code and performance test and missed that. I'll update that and file a CSR. 
> Thanks.

CSR filed [1]

Thanks,

Brian

[1] https://bugs.openjdk.java.net/browse/JDK-8256698

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[Roger Riggs]

On Thu, 19 Nov 2020 20:20:17 GMT, Brian Burkhalter  wrote:

>> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
>> improve its performance when `skip(long)` skips fewer than the requested 
>> number of bytes. In the current implementation, `skip(long)` is invoked once 
>> and, if not enough bytes have been skipped, then `read()` is invoked for 
>> each of the remaining bytes to be skipped. The proposed implementation 
>> instead repeatedly invokes `skip(long)` until the requested number of bytes 
>> has been skipped, or an error condition is encountered. For cases where 
>> `skip(long)` skips fewer bytes than the number requested, the new version 
>> was measured to be up to more than one thousand times faster than the old 
>> version. When `skip(long)` actually skips the requested number of bytes, the 
>> performance difference is insignificant.
>
> Brian Burkhalter has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently
>  - 8246739: InputStream.skipNBytes could be implemented more efficiently

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently [v2]

[Brian Burkhalter]

> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
> improve its performance when `skip(long)` skips fewer than the requested 
> number of bytes. In the current implementation, `skip(long)` is invoked once 
> and, if not enough bytes have been skipped, then `read()` is invoked for each 
> of the remaining bytes to be skipped. The proposed implementation instead 
> repeatedly invokes `skip(long)` until the requested number of bytes has been 
> skipped, or an error condition is encountered. For cases where `skip(long)` 
> skips fewer bytes than the number requested, the new version was measured to 
> be up to more than one thousand times faster than the old version. When 
> `skip(long)` actually skips the requested number of bytes, the performance 
> difference is insignificant.

Brian Burkhalter has updated the pull request incrementally with two additional 
commits since the last revision:

 - 8246739: InputStream.skipNBytes could be implemented more efficiently
 - 8246739: InputStream.skipNBytes could be implemented more efficiently

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1329/files
  - new: https://git.openjdk.java.net/jdk/pull/1329/files/331f3fcc..f59d7a35

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1329=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1329=00-01

  Stats: 8 lines in 1 file changed: 1 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1329.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1329/head:pull/1329

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently

[Brian Burkhalter]

On Thu, 19 Nov 2020 19:58:16 GMT, Naoto Sato  wrote:

> The code looks good. Should we also change the @implSpec, as it specifically 
> describes how it works, i.e., calling no-arg read(), but the new impl calls 
> read() with buffer.

Good catch. Yes, the `@implSpec` should be updated. I was too focused on the 
code and performance test and missed that. I'll update that and file a CSR. 
Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently

[Sergey Bylokhov]

On Thu, 19 Nov 2020 19:29:43 GMT, Brian Burkhalter  wrote:

> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
> improve its performance when `skip(long)` skips fewer than the requested 
> number of bytes. In the current implementation, `skip(long)` is invoked once 
> and, if not enough bytes have been skipped, then `read()` is invoked for each 
> of the remaining bytes to be skipped. The proposed implementation instead 
> repeatedly invokes `skip(long)` until the requested number of bytes has been 
> skipped, or an error condition is encountered. For cases where `skip(long)` 
> skips fewer bytes than the number requested, the new version was measured to 
> be up to more than one thousand times faster than the old version. When 
> `skip(long)` actually skips the requested number of bytes, the performance 
> difference is insignificant.

src/java.base/share/classes/java/io/InputStream.java line 596:

> 594:  * @seejava.io.InputStream#skip(long)
> 595:  */
> 596: public void skipNBytes(long n) throws IOException {

Not related to this change, but looks like `@since` is missing.

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently

[Brian Burkhalter]

On Thu, 19 Nov 2020 19:52:05 GMT, Sergey Bylokhov  wrote:

>> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
>> improve its performance when `skip(long)` skips fewer than the requested 
>> number of bytes. In the current implementation, `skip(long)` is invoked once 
>> and, if not enough bytes have been skipped, then `read()` is invoked for 
>> each of the remaining bytes to be skipped. The proposed implementation 
>> instead repeatedly invokes `skip(long)` until the requested number of bytes 
>> has been skipped, or an error condition is encountered. For cases where 
>> `skip(long)` skips fewer bytes than the number requested, the new version 
>> was measured to be up to more than one thousand times faster than the old 
>> version. When `skip(long)` actually skips the requested number of bytes, the 
>> performance difference is insignificant.
>
> src/java.base/share/classes/java/io/InputStream.java line 596:
> 
>> 594:  * @seejava.io.InputStream#skip(long)
>> 595:  */
>> 596: public void skipNBytes(long n) throws IOException {
> 
> Not related to this change, but looks like `@since` is missing.

https://bugs.openjdk.java.net/browse/JDK-8256183

-

PR: https://git.openjdk.java.net/jdk/pull/1329

Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently

[Naoto Sato]

On Thu, 19 Nov 2020 19:29:43 GMT, Brian Burkhalter  wrote:

> Please review this modification of `java.io.InputStream.skipNBytes(long)` to 
> improve its performance when `skip(long)` skips fewer than the requested 
> number of bytes. In the current implementation, `skip(long)` is invoked once 
> and, if not enough bytes have been skipped, then `read()` is invoked for each 
> of the remaining bytes to be skipped. The proposed implementation instead 
> repeatedly invokes `skip(long)` until the requested number of bytes has been 
> skipped, or an error condition is encountered. For cases where `skip(long)` 
> skips fewer bytes than the number requested, the new version was measured to 
> be up to more than one thousand times faster than the old version. When 
> `skip(long)` actually skips the requested number of bytes, the performance 
> difference is insignificant.

The code looks good. Should we also change the @implSpec, as it specifically 
describes how it works, i.e., calling no-arg read(), but the new impl calls 
read() with buffer.

-

PR: https://git.openjdk.java.net/jdk/pull/1329