On Thu, 6 Apr 2023 09:55:57 GMT, Sergey Tsypanov wrote:
>>> So I think the main thing now is to decide whether the benchmark should be
>>> included or not.
>>
>> In my review I hadn't paid attention to the benchmark class. I now applied
>> this patch locally and reviewed the benchmark code and
On Thu, 6 Apr 2023 10:07:31 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `B
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Thu, 6 Apr 2023 09:23:33 GMT, Jaikiran Pai wrote:
> For this change, I think a benchmark isn't necessary - what would it test,
> how quickly a new BufferedInputStream returns as compared to previous code?
@jaikiran first of all it demonstrates reducing allocation for plain/cascaded
readAll
On Wed, 5 Apr 2023 07:08:58 GMT, Alan Bateman wrote:
> So I think the main thing now is to decide whether the benchmark should be
> included or not.
In my review I hadn't paid attention to the benchmark class. I now applied this
patch locally and reviewed the benchmark code and even ran it loc
On Mon, 27 Mar 2023 16:18:22 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Mon, 27 Mar 2023 16:18:22 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Mon, 27 Mar 2023 16:18:22 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Mon, 27 Mar 2023 16:18:22 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Mon, 27 Mar 2023 15:24:05 GMT, Brian Burkhalter wrote:
>> I skimmed through the latest version but there are still several issues.
>> Can you try the patch below instead? This will ensure that read, skip,
>> reset, etc. check buf as before. It also ensures that read1 does the same
>> thres
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Fri, 24 Mar 2023 19:30:22 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Fri, 24 Mar 2023 10:27:45 GMT, Alan Bateman wrote:
> Can you try the patch below instead? This will ensure that read, skip, reset,
> etc. check buf as before.
I think that the implementation provided by @AlanBateman looks good.
-
PR Comment: https://git.openjdk.org/jdk/pull/131
On Sat, 25 Mar 2023 05:55:46 GMT, Eirik Bjorsnos wrote:
> I think the benchmark would benefit with a few words explaining why you
> focused on those methods/calling patterns. For someone walking past this
> benchmark, this is not obvious why you picked those.
I think the interesting case is wh
On Fri, 24 Mar 2023 19:30:22 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Fri, 24 Mar 2023 17:59:24 GMT, Chen Liang wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8304745: Amend copyright
>
> test/micro/org/openjdk/bench/java/io/BufferedInputStreamBenchmark.java line 1:
>
>> 1: /
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Fri, 24 Mar 2023 17:02:30 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Fri, 24 Mar 2023 17:51:03 GMT, Eirik Bjorsnos wrote:
> Is something wrong with line endings in the added benchmark? jcheck seems to
> complain a lot?
It often happens when copying a file on windows. Can usually be fixed in
intellij idea by navigating to the file and changing line endings to
On Fri, 24 Mar 2023 17:02:30 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Fri, 24 Mar 2023 16:12:13 GMT, Chen Liang wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8304745: Add benchmark
>
> test/micro/org/openjdk/bench/java/io/BufferedInputStreamBenchmark.java line 2:
>
>> 1: /*
On Fri, 24 Mar 2023 15:40:40 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Thu, 23 Mar 2023 21:15:44 GMT, Eirik Bjorsnos wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.base/share/classes/java/io/BufferedInputStream.java
>>
>> Co-authored-by: liach <7806504+li..
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Fri, 24 Mar 2023 10:27:45 GMT, Alan Bateman wrote:
> This will ensure that read, skip, reset, etc. check buf as before
Why do we so heavily rely on `buf` if we can do open/close check with
`getInIfOpen()`?
-
PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-148266
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Fri, 24 Mar 2023 06:40:24 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Thu, 23 Mar 2023 19:48:40 GMT, Eirik Bjorsnos wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.base/share/classes/java/io/BufferedInputStream.java
>>
>> Co-authored-by: liach <7806504+li..
On Thu, 23 Mar 2023 19:48:40 GMT, Eirik Bjorsnos wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.base/share/classes/java/io/BufferedInputStream.java
>>
>> Co-authored-by: liach <7806504+li..
On Thu, 23 Mar 2023 19:55:04 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/BufferedInputStream.java line 183:
>>
>>> 181: if (buffer == EMPTY) {
>>> 182: buf = buffer = new byte[size];
>>> 183: }
>>
>> You should probably use compareAndSet here too
On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Thu, 23 Mar 2023 19:41:24 GMT, Daniel Fuchs wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.base/share/classes/java/io/BufferedInputStream.java
>>
>> Co-authored-by: liach <7806504+li...@
On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Thu, 23 Mar 2023 19:53:13 GMT, Chen Liang wrote:
> The lazy creation only happens if it's the exact BufferedInputStream class,
> which is already handled like the internal lock.
Maybe, but how does it play with asynchronous close?
-
PR Comment: https://git.openjdk.org/jdk/pull/
On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with
>> capacity 8192. In some cases this buffer is never used, e.g. when we call
>> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
>> `
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `BufferedInputStream` is cascaded.
Sergey Tsypanov has updated
On Thu, 23 Mar 2023 14:51:40 GMT, Sergey Tsypanov wrote:
>> src/java.base/share/classes/java/io/BufferedInputStream.java line 211:
>>
>>> 209: throw new IllegalArgumentException("Buffer size <= 0");
>>> 210: }
>>> 211: buf = new byte[0];
>>
>> Two recommendations:
>>
On Wed, 22 Mar 2023 23:01:32 GMT, Chen Liang wrote:
> This change exposes the CLOSED array
Otherwise we have to either call `getInIfOpen()`/`getBufIfOpen` making them at
least protected or dodge this somehow via `SharedSecrets`
> I mean that additional API exposure is acceptable and backward c
On Thu, 23 Mar 2023 15:18:54 GMT, Chen Liang wrote:
>> But it's just comment clean up
>
> No? The one above is API specification (Javadoc); the one below is comments.
Right, I'll revert it
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146370513
On Thu, 23 Mar 2023 14:48:32 GMT, Sergey Tsypanov wrote:
>> src/java.base/share/classes/java/io/BufferedInputStream.java line 78:
>>
>>> 76: private final InternalLock lock;
>>> 77:
>>> 78: /**
>>
>> These should be reverted, now that the subclasses will see the same behavior
>> as be
On Thu, 23 Mar 2023 15:06:46 GMT, Sergey Tsypanov wrote:
>> Good point, done!
>
> Btw, maybe it'd be better to rename `getBufIfOpen()` into e.g. `getBuffer()`
> to make it less confusing?
I'd say feel free to rename these private methods. I believe this patch is
mature enough for public review
On Wed, 22 Mar 2023 20:34:08 GMT, Sergey Tsypanov wrote:
> By default `BufferedInputStream` is constructed with internal buffer with
> capacity 8192. In some cases this buffer is never used, e.g. when we call
> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
> `Buff
By default `BufferedInputStream` is constructed with internal buffer with
capacity 8192. In some cases this buffer is never used, e.g. when we call
`IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when
`BufferedInputStream` is cascaded.
-
Commit messages:
- 8
49 matches
Mail list logo
