Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Sat, 23 Dec 2023 13:01:09 GMT, Markus KARG wrote: >Typically it was sufficient to document the JMH results before and after a PR >just once (not dynamically in the form of a test). The problem with "results before and after a PR just once" is those benchmarks will not automatically fail the build if somebody else breaks the optimization unintentionally. In other words, benchmarks do not serve as a safety net. At the same time, if the test code were to ensure "less than 20 bytes allocated" in a common case of transferring data from `BufferedInputStream` to `ByteArrayOutputStream`, then it would be a nice safety net so the optimization will not be broken unintentionally. >The latter should be rather easy to check: Invoke transferTo(out) two times in >a row and compare the identity of the two byte arrays passed to out.write(). >If they stay the same, then apparently no temporary copy was created. Two >achieve this, the BIS must be wrapper around an extendable input stream (like >FileInputStream) so between calls the stream could get extended (e. g. by >writing into the file) Markus, I am afraid you missed that anything that "extends FileOutputStream" (I assume you mean `FileOutputStream`, not `FileInputStream`) would **not** qualify as a trusted stream, and `transferTo` will make a copy. The current check is `clazz == FileOutputStream.class` - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435625914
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Sat, 23 Dec 2023 15:02:28 GMT, Vladimir Sitnikov wrote: >> Why do you want to put *that* in a test case at all? So far I did not come >> across performance-based *tests* (only *benchmarks*), i. e. nobody ever >> requested that from me in any of my NIO optimizations. Typically it was >> sufficient to document the JMH results before and after a PR *just once* >> (not dynamically in the form of a test). >> >> For *this* PR it is sufficient to proof what the PR's title says: Proof the >> *direct* call (i. e. the *absence of a copy*) and we're good to merge. I >> already explained how to proof *that*. > >>Typically it was sufficient to document the JMH results before and after a PR >>just once (not dynamically in the form of a test). > > The problem with "results before and after a PR just once" is those > benchmarks will not automatically fail the build if somebody else breaks the > optimization unintentionally. In other words, benchmarks do not serve as a > safety net. > > At the same time, if the test code were to ensure "less than 20 bytes > allocated" in a common case of transferring data from `BufferedInputStream` > to `ByteArrayOutputStream`, then it would be a nice safety net so the > optimization will not be broken unintentionally. > >>The latter should be rather easy to check: Invoke transferTo(out) two times >>in a row and compare the identity of the two byte arrays passed to >>out.write(). If they stay the same, then apparently no temporary copy was >>created. Two achieve this, the BIS must be wrapper around an extendable input >>stream (like FileInputStream) so between calls the stream could get extended >>(e. g. by writing into the file) > > Markus, I am afraid you missed that anything that "extends FileOutputStream" > (I assume you mean `FileOutputStream`, not `FileInputStream`) would **not** > qualify as a trusted stream, and `transferTo` will make a copy. The current > check is `clazz == FileOutputStream.class` I think we should not overcomplicate things. Nobody really asked for actually failing the test when either the number of bytes allocated or the time to allocate it execeeds an expected limit. The requirement for the test solely is that it proofs the claim outlined by the topic: Proof that BufferedInputStream.bug is used directly and fail if not. Nothing else. Everthing beyond that is nice to have as we did not have such a test for the similar change at BAIS. As long as it is proven that we use *the same* buffer always, it is implicitly proven that the original claim of the published benchmarks still holds true. There is no need to re-evaluate the actual benchmark again and again, it will not provide any additional benefit but would make the test much more complex, less reliable, and increase the risk of false-fail in automated test farms using alternating hardware. I did not mean `FileOutputStream` but really `FileInputStream`: You have a file as an input, so you can extend the file between two calls to `transferTo(out)` to be actually able to execute `transferTo` two times in a row. Nevertheless you are right, the problem with my proposed solution is that the mock receiver needed in that scenario (the code that records the identity of the buffer) would not be in the set of trusted classes. Actually I do not see how we can solve this. @bplb In https://github.com/openjdk/jdk/pull/16893 we actually did not check if the PR is actually effective, but solely if the buffer is effectively unchanged. The same could be done here. Is it OK for you if we skip checking the actual efficiency of the *actual target* of this PR itself (i. e. still not checking if it is *the same* buffer)? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435679827
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 15:08:33 GMT, Markus KARG wrote: > IMHO the trigger for this PR was sparing time That is fair. Do you know how one could create a reliable test for performance that would fail without the fix and succeed with the fix? I think the allocation is a reasonable proxy for the duration in this case, and the allocation is more-or-less easy to measure and test if the JVM supports `getCurrentThreadAllocatedBytes`. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435200386
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 16:25:29 GMT, Vladimir Sitnikov wrote: >> IMHO the trigger for this PR was sparing *time*, not necessarily sparing >> *bytes* (the default buffer size is just 8K); the latter certainly is a nice >> and beneficial side effect. But I may be wrong here, then the original >> contributor should chime in now and clarify. > >> IMHO the trigger for this PR was sparing time > > That is fair. Do you know how one could create a reliable test for > performance that would fail without the fix and succeed with the fix? > > I think the allocation is a reasonable proxy for the duration in this case, > and the allocation is more-or-less easy to measure and test if the JVM > supports `getCurrentThreadAllocatedBytes`. Why do you want to put *that* in a test case at all? So far I did not come across performance-based *tests* (only *benchmarks*), i. e. nobody ever requested that from me in any of my NIO optimizations. Typically it was sufficient to document the JMH results before and after a PR *just once* (not dynamically in the form of a test). For *this* PR it is sufficient to proof what the PR's title says: Proof the *direct* call (i. e. the *absence of a copy*) and we're good to merge. I already explained how to proof *that*. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435596696
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 14:37:26 GMT, Vladimir Sitnikov wrote: >> I think there is a misundertanding. This PR is not intendend to reduce the >> *amount* of allocated heap, it is about sparing time by not creating >> *temporary copies*. The latter should be rather easy to check: Invoke >> `transferTo(out)` two times in a row and compare the *identity* of the two >> byte arrays passed to `out.write()`. If they stay the same, then apparently >> no *temporary copy* was created. Two achieve this, the BIS must be wrapper >> around an extendable input stream (like `FileInputStream`) so between calls >> the stream could get extended (e. g. by writing into the file). > > Markus, could you please double-check? A temporary allocation *is* an > allocation, and it is the point of the change to avoid allocation and copying > the temp data when the output stream is known to behave well. I am sure the > allocation "before the change" would be non-trivial (more than several KiB), > and after the change the allocation within .transferTo would be minimal (few > bytes) assuming the output stream has already allocated its buffer IMHO the trigger for this PR was sparing *time*, not necessarily sparing *bytes* (the default buffer size is just 8K); the latter certainly is a nice and beneficial side effect. But I may be wrong here, then the original contributor should chime in now and clarify. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435146173
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 14:27:16 GMT, Markus KARG wrote: >> Can anybody give a hint how one can create assertions in OpenJDK test code >> that would check the amount of allocated heap for the tested method? >> >> Since the change here is "removal of an allocation", the assert in the code >> should probably allow only a very small allocation in `.transferTo`. >> >> Does `com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes()` >> sound right for the test? > > I think there is a misundertanding. This PR is not intendend to reduce the > *amount* of allocated heap, it is about sparing time by not creating > *temporary copies*. The latter should be rather easy to check: Invoke > `transferTo(out)` two times in a row and compare the *identity* of the two > byte arrays passed to `out.write()`. If they stay the same, then apparently > no *temporary copy* was created. Two achieve this, the BIS must be wrapper > around an extendable input stream (like `FileInputStream`) so between calls > the stream could get extended (e. g. by writing into the file). Markus, could you please double-check? A temporary allocation *is* an allocation, and it is the point of the change to avoid allocation and copying the temp data when the output stream is known to behave well. I am sure the allocation "before the change" would be non-trivial (more than several KiB), and after the change the allocation within .transferTo would be minimal (few bytes) assuming the output stream has already allocated its buffer - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435120325
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 13:53:42 GMT, Vladimir Sitnikov wrote: >> test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 68: >> >>> 66: >>> 67: var bis = new BufferedInputStream(new >>> ByteArrayInputStream(dup)); >>> 68: bis.mark(dup.length); >> >> If you take a closer look into `BIS::transferTo()` then you will notice that >> in case you call `bis.mark()` then your optimization will effectively *not >> getting called at all*, due to this line: >> https://github.com/openjdk/jdk/pull/16879/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1L643. >> So Brian's comment still applies and you should fix your test before this >> PR can be accepted for a merge (hence: you MUST get rid of `mark()`). > > Can anybody give a hint how one can create assertions in OpenJDK test code > that would check the amount of allocated heap for the tested method? > > Since the change here is "removal of an allocation", the assert in the code > should probably allow only a very small allocation in `.transferTo`. > > Does `com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes()` sound > right for the test? I think there is a misundertanding. This PR is not intendend to reduce the *amount* of allocated heap, it is about sparing time by not creating *temporary copies*. The latter should be rather easy to check: Invoke `transferTo(out)` two times in a row and compare the *identity* of the two byte arrays passed to `out.write()`. If they stay the same, then apparently no *temporary copy* was created. Two achieve this, the BIS must be wrapper around an extendable input stream (like `FileInputStream`) so between calls the stream could get extended (e. g. by writing into the file). - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435108416
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 13:19:46 GMT, Markus KARG wrote: >> Sergey Tsypanov has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 22 additional >> commits since the last revision: >> >> - Merge branch 'master' into 8320971 >> - 8320971: Inline tests >> - 8322292: Remove TransferToTrusted.checkedOutputStream() >> - 8320971: Adjust JavaDoc >> - Merge branch 'master' into 8320971 >> - 8320971: Revert irrelevant changes >> - 8320971: Add more tests >> - 8320971: Fix JavaDoc >> - 8320971: Whitespaces >> - 8320971: Fix build >> - ... and 12 more: https://git.openjdk.org/jdk/compare/d38d95d8...ee035998 > > test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 68: > >> 66: >> 67: var bis = new BufferedInputStream(new ByteArrayInputStream(dup)); >> 68: bis.mark(dup.length); > > If you take a closer look into `BIS::transferTo()` then you will notice that > in case you call `bis.mark()` then your optimization will effectively *not > getting called at all*, due to this line: > https://github.com/openjdk/jdk/pull/16879/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1L643. > So Brian's comment still applies and you should fix your test before this PR > can be accepted for a merge (hence: you MUST get rid of `mark()`). Can anybody give a hint how one can create assertions in OpenJDK test code that would check the amount of allocated heap for the tested method? Since the change here is "removal of an allocation", the assert in the code should probably allow only a very small allocation in `.transferTo`. Does `com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes()` sound right for the test? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435074076
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 09:55:15 GMT, Sergey Tsypanov wrote: >> It looks like we can skip copying of `byte[]` in >> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in >> `java.io`. >> >> See comment by @vlsi in >> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 > > Sergey Tsypanov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 22 additional > commits since the last revision: > > - Merge branch 'master' into 8320971 > - 8320971: Inline tests > - 8322292: Remove TransferToTrusted.checkedOutputStream() > - 8320971: Adjust JavaDoc > - Merge branch 'master' into 8320971 > - 8320971: Revert irrelevant changes > - 8320971: Add more tests > - 8320971: Fix JavaDoc > - 8320971: Whitespaces > - 8320971: Fix build > - ... and 12 more: https://git.openjdk.org/jdk/compare/17ab55e8...ee035998 test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 68: > 66: > 67: var bis = new BufferedInputStream(new ByteArrayInputStream(dup)); > 68: bis.mark(dup.length); If you take a closer look into `BIS::transferTo()` then you will notice that in case you call `bis.mark()` then your optimization will effectively *not getting called at all*, due to this line: https://github.com/openjdk/jdk/pull/16879/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1L643. So Brian's comment still applies and you should fix your test before this PR can be accepted for a merge (hence: you MUST get rid of `mark()`). - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435050559
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
> It looks like we can skip copying of `byte[]` in > `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in > `java.io`. > > See comment by @vlsi in > https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 Sergey Tsypanov has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 22 additional commits since the last revision: - Merge branch 'master' into 8320971 - 8320971: Inline tests - 8322292: Remove TransferToTrusted.checkedOutputStream() - 8320971: Adjust JavaDoc - Merge branch 'master' into 8320971 - 8320971: Revert irrelevant changes - 8320971: Add more tests - 8320971: Fix JavaDoc - 8320971: Whitespaces - 8320971: Fix build - ... and 12 more: https://git.openjdk.org/jdk/compare/15c2671f...ee035998 - Changes: - all: https://git.openjdk.org/jdk/pull/16879/files - new: https://git.openjdk.org/jdk/pull/16879/files/84686bc6..ee035998 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16879=16 - incr: https://webrevs.openjdk.org/?repo=jdk=16879=15-16 Stats: 1284 lines in 66 files changed: 844 ins; 243 del; 197 mod Patch: https://git.openjdk.org/jdk/pull/16879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16879/head:pull/16879 PR: https://git.openjdk.org/jdk/pull/16879
