Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]
On Wed, 22 May 2024 07:49:27 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change for addressing >> https://bugs.openjdk.org/browse/JDK-8332490? >> >> The jmh test opens a `InflaterInputStream`, reads the stream contents, but >> then doesn't close the stream. This can lead to resource leak which can then >> cause OOM as noted in that JBS issue. >> >> The commit in this PR closes the `InflaterInputStream` when the reads >> complete. >> >> >> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams >> >> and >> >> >> ./build/macosx-aarch64/images/jdk/bin/java -jar >> ./build/macosx-aarch64/images/test/micro/benchmarks.jar >> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead >> -t max -f 1 -wi 2 -foe true >> >> pass after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > add a comment Thank you Alan and Claes for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/19340#issuecomment-2126009374
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]
On Wed, 22 May 2024 07:49:27 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change for addressing >> https://bugs.openjdk.org/browse/JDK-8332490? >> >> The jmh test opens a `InflaterInputStream`, reads the stream contents, but >> then doesn't close the stream. This can lead to resource leak which can then >> cause OOM as noted in that JBS issue. >> >> The commit in this PR closes the `InflaterInputStream` when the reads >> complete. >> >> >> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams >> >> and >> >> >> ./build/macosx-aarch64/images/jdk/bin/java -jar >> ./build/macosx-aarch64/images/test/micro/benchmarks.jar >> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead >> -t max -f 1 -wi 2 -foe true >> >> pass after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > add a comment Sorry, thought I had already approved. Comment is fine. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2072382952
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]
On Wed, 22 May 2024 07:28:04 GMT, Claes Redestad wrote: > LGTM - feel free to add a comment that closing the InflaterInputStream has no > effect on the underlying stream deflated. Hello Claes, I've updated the PR to include a comment. - PR Comment: https://git.openjdk.org/jdk/pull/19340#issuecomment-2124098078
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]
On Wed, 22 May 2024 07:27:14 GMT, Claes Redestad wrote:
>> test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line
>> 113:
>>
>>> 111: try (InflaterInputStream iis = new
>>> InflaterInputStream(deflated)) {
>>> 112: while (iis.read(inflated, 0, inflated.length) != -1);
>>> 113: }
>>
>> Presumably this only works because closing the underlying stream (a BAIS in
>> this case) is a no-op.
>
> Yes, the underlying BAIS can be repeatedly closed without affecting the
> benchmark.
Hello Alan, before this change, the memory gets occupied by too many of these:
1: 10871820 521847360
jdk.internal.ref.CleanerImpl$PhantomCleanableRef (java.base@23-internal)
2: 10871758 260922192 java.util.zip.Inflater$InflaterZStreamRef
(java.base@23-internal)
Closing the `InflaterInputStream` releases those underlying
`InflaterZStreamRef`s.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609461029
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]
> Can I please get a review of this test-only change for addressing > https://bugs.openjdk.org/browse/JDK-8332490? > > The jmh test opens a `InflaterInputStream`, reads the stream contents, but > then doesn't close the stream. This can lead to resource leak which can then > cause OOM as noted in that JBS issue. > > The commit in this PR closes the `InflaterInputStream` when the reads > complete. > > > make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams > > and > > > ./build/macosx-aarch64/images/jdk/bin/java -jar > ./build/macosx-aarch64/images/test/micro/benchmarks.jar > org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead > -t max -f 1 -wi 2 -foe true > > pass after this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: add a comment - Changes: - all: https://git.openjdk.org/jdk/pull/19340/files - new: https://git.openjdk.org/jdk/pull/19340/files/f8687c3f..42a2d32a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19340&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19340&range=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19340.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19340/head:pull/19340 PR: https://git.openjdk.org/jdk/pull/19340
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
On Wed, 22 May 2024 07:20:04 GMT, Alan Bateman wrote:
>> Can I please get a review of this test-only change for addressing
>> https://bugs.openjdk.org/browse/JDK-8332490?
>>
>> The jmh test opens a `InflaterInputStream`, reads the stream contents, but
>> then doesn't close the stream. This can lead to resource leak which can then
>> cause OOM as noted in that JBS issue.
>>
>> The commit in this PR closes the `InflaterInputStream` when the reads
>> complete.
>>
>>
>> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>>
>> and
>>
>>
>> ./build/macosx-aarch64/images/jdk/bin/java -jar
>> ./build/macosx-aarch64/images/test/micro/benchmarks.jar
>> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead
>> -t max -f 1 -wi 2 -foe true
>>
>> pass after this change.
>
> test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 113:
>
>> 111: try (InflaterInputStream iis = new
>> InflaterInputStream(deflated)) {
>> 112: while (iis.read(inflated, 0, inflated.length) != -1);
>> 113: }
>
> Presumably this only works because closing the underlying stream (a BAOS in
> this case) is a no-op.
Yes, the underlying BAIS can be repeatedly closed without affecting the
benchmark.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609436837
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change for addressing > https://bugs.openjdk.org/browse/JDK-8332490? > > The jmh test opens a `InflaterInputStream`, reads the stream contents, but > then doesn't close the stream. This can lead to resource leak which can then > cause OOM as noted in that JBS issue. > > The commit in this PR closes the `InflaterInputStream` when the reads > complete. > > > make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams > > and > > > ./build/macosx-aarch64/images/jdk/bin/java -jar > ./build/macosx-aarch64/images/test/micro/benchmarks.jar > org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead > -t max -f 1 -wi 2 -foe true > > pass after this change. LGTM - feel free to add a comment that closing the `InflaterInputStream` has no effect on the underlying stream `deflated`. - PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2070357036
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai wrote:
> Can I please get a review of this test-only change for addressing
> https://bugs.openjdk.org/browse/JDK-8332490?
>
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but
> then doesn't close the stream. This can lead to resource leak which can then
> cause OOM as noted in that JBS issue.
>
> The commit in this PR closes the `InflaterInputStream` when the reads
> complete.
>
>
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>
> and
>
>
> ./build/macosx-aarch64/images/jdk/bin/java -jar
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead
> -t max -f 1 -wi 2 -foe true
>
> pass after this change.
test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 113:
> 111: try (InflaterInputStream iis = new
> InflaterInputStream(deflated)) {
> 112: while (iis.read(inflated, 0, inflated.length) != -1);
> 113: }
Presumably this only works because closing the underlying stream (a BAOS in
this case) is a no-op.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609427551
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change for addressing > https://bugs.openjdk.org/browse/JDK-8332490? > > The jmh test opens a `InflaterInputStream`, reads the stream contents, but > then doesn't close the stream. This can lead to resource leak which can then > cause OOM as noted in that JBS issue. > > The commit in this PR closes the `InflaterInputStream` when the reads > complete. > > > make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams > > and > > > ./build/macosx-aarch64/images/jdk/bin/java -jar > ./build/macosx-aarch64/images/test/micro/benchmarks.jar > org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead > -t max -f 1 -wi 2 -foe true > > pass after this change. Marked as reviewed by aturbanov (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2070259473
