Integrated: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
This pull request has now been integrated.
Changeset: ec2fc384
Author:Sergey Bylokhov
URL:
https://git.openjdk.java.net/jdk/commit/ec2fc384e50668b667335f973ffeb5a19bbcfb9b
Stats: 127 lines in 15 files changed: 24 ins; 53 del; 50 mod
8272120: Avoid looking for standard encodings in "java." modules
Reviewed-by: alanb, dfuchs, naoto
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance
On Thu, 12 Aug 2021 01:16:04 GMT, Brian Burkhalter wrote: > Please consider this request to add an override > `java.io.FileInputStream.transferTo(OutputStream)` with improved performance > if the parameter is a `FileOutputStream`. Invoking `transferTo()` on a `FileInputStream` will use the superclass method which copies using looping over buffers of a fixed size. If the parameter is a `FileOutputStream`, the copy is greatly accelerated by using the `java.nio.channels.FileChannel.transferTo()` method with the `FileChannel`s of the source and destination streams. Performance measurements were made for streams of length 10, 100, 1, and 10 bytes. Throughput improvement was observed for all cases on the three usual platforms, with the largest of nearly 5X being seen on Linux because in this case the sendfile() system call is used underneath. In no case did throughput decrease. - PR: https://git.openjdk.java.net/jdk/pull/5097
RFR: 8272297: FileInputStream should override transferTo() for better performance
Please consider this request to add an override `java.io.FileInputStream.transferTo(OutputStream)` with improved performance if the parameter is a `FileOutputStream`. - Commit messages: - 8272297: FileInputStream should override transferTo() for better performance Changes: https://git.openjdk.java.net/jdk/pull/5097/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5097=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272297 Stats: 21 lines in 1 file changed: 21 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5097/head:pull/5097 PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG
wrote:
>> This PR-*draft* is **work in progress** and an invitation to discuss a
>> possible solution for issue
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not
>> yet* intended for a final review.
>>
>> As proposed in JDK-8265891, this PR provides an implementation for
>> `Channels.newInputStream().transferTo()` which provide superior performance
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading
>> to deeper levels via NIO, hence allowing the operating system to optimize
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern
>> hardware / fast I/O devides.
>>
>> Using JMH I have benchmarked both, the original implementation and this
>> implementation, and (depending on the used hardware and use case)
>> performance change was approx. doubled performance. So this PoC proofs that
>> it makes sense to finalize this work and turn it into an actual OpenJDK
>> contribution.
>>
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Draft: Eliminated duplicate code using lambda expressions
> - Draft: Use blocking mode also for target channel
src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 175:
> 173: throw new IllegalBlockingModeException();
> 174: }
> 175: return tls.supply();
It does not look like the `SelectableChannel` branch is tested, including
whether an `IllegalBlockingModeException` is thrown when it should be.
src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 249:
> 247: }
> 248:
> 249: private static long transfer(ReadableByteChannel src,
> WritableByteChannel dst) throws IOException {
Does this method have a measurable improvement in performance over
`InputStream.transferTo()`?
test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 93:
> 91: checkTransferredContents(inputStreamProvider,
> outputStreamProvider, createRandomBytes(1024, 4096));
> 92: // to span through several batches
> 93: checkTransferredContents(inputStreamProvider,
> outputStreamProvider, createRandomBytes(16384, 16384));
Should some random-sized buffers be tested? (Use `jdk.test.lib.RandomFactory`
factory for this, not `j.u.Random`. The existing use of `Random` is fine.)
Should some testing be done of streams with non-zero initial position?
test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 101:
> 99: try (InputStream in = inputStreamProvider.input(inBytes);
> 100: OutputStream out =
> outputStreamProvider.output(recorder::set)) {
> 101: in.transferTo(out);
The return value of `transferTo()` is not checked.
-
PR: https://git.openjdk.java.net/jdk/pull/4263
Integrated: 8271170: Add unit test for what jpackage app launcher puts in the environment
On Mon, 9 Aug 2021 17:02:55 GMT, Alexey Semenyuk wrote: > Added jtreg test to verify jpackage launcher adds app dir to > `java.library.path` system property. > Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. > Moved test/jdk/tools/jpackage/apps/image/Hello.java to > test/jdk/tools/jpackage/apps/Hello.java. This pull request has now been integrated. Changeset: 44f137ff Author:Alexey Semenyuk URL: https://git.openjdk.java.net/jdk/commit/44f137ff9c0229ab2d5eccd9ebaadf8db11f386d Stats: 330 lines in 7 files changed: 200 ins; 116 del; 14 mod 8271170: Add unit test for what jpackage app launcher puts in the environment Reviewed-by: almatvee, herrick - PR: https://git.openjdk.java.net/jdk/pull/5056
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking [v4]
On Wed, 11 Aug 2021 21:40:52 GMT, Claes Redestad wrote: >> In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to >> avoid potentially getting two bounds checks in the UTF-16 case. Problem is >> this optimization cause a regression since `StringUTF16.charAt(..)` checks >> `index` against the length of the `value` array and not `count`. >> >> A correct fix that still maintain the possible performance advantage >> reported by #4738 is to reinstate the `checkIndex(value, count)` and call >> `StringUTF16.getChar` instead of `charAt`. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copy-paste error Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking [v4]
> In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fix copy-paste error - Changes: - all: https://git.openjdk.java.net/jdk/pull/5086/files - new: https://git.openjdk.java.net/jdk/pull/5086/files/4ef9ccbc..83a64828 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5086=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5086=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5086.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5086/head:pull/5086 PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking [v3]
> In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Fix copy-paste error - Add UTF-16 tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/5086/files - new: https://git.openjdk.java.net/jdk/pull/5086/files/e29c576a..4ef9ccbc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5086=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5086=01-02 Stats: 23 lines in 1 file changed: 18 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5086.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5086/head:pull/5086 PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment [v4]
> Added jtreg test to verify jpackage launcher adds app dir to > `java.library.path` system property. > Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. > Moved test/jdk/tools/jpackage/apps/image/Hello.java to > test/jdk/tools/jpackage/apps/Hello.java. Alexey Semenyuk has updated the pull request incrementally with one additional commit since the last revision: dummy commit - Changes: - all: https://git.openjdk.java.net/jdk/pull/5056/files - new: https://git.openjdk.java.net/jdk/pull/5056/files/27e82df3..3e7011e7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5056=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5056=02-03 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5056/head:pull/5056 PR: https://git.openjdk.java.net/jdk/pull/5056
Integrated: 8272328: java.library.path is not set properly by Windows jpackage app launcher
On Wed, 11 Aug 2021 17:52:57 GMT, Alexey Semenyuk wrote: > Restart window app launcher to make changes made to `PATH` env variable > visible to JLI code in > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L347 This pull request has now been integrated. Changeset: cd2dbe5f Author:Alexey Semenyuk URL: https://git.openjdk.java.net/jdk/commit/cd2dbe5f007baf81ae9262c1152917e620970621 Stats: 75 lines in 4 files changed: 63 ins; 5 del; 7 mod 8272328: java.library.path is not set properly by Windows jpackage app launcher Reviewed-by: herrick, almatvee - PR: https://git.openjdk.java.net/jdk/pull/5090
Re: RFR: 8272328: java.library.path is not set properly by Windows jpackage app launcher
On Wed, 11 Aug 2021 17:52:57 GMT, Alexey Semenyuk wrote: > Restart window app launcher to make changes made to `PATH` env variable > visible to JLI code in > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L347 Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5090
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking [v2]
> In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add SB.charAt test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5086/files - new: https://git.openjdk.java.net/jdk/pull/5086/files/e72d9e15..e29c576a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5086=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5086=00-01 Stats: 66 lines in 1 file changed: 66 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5086.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5086/head:pull/5086 PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8272328: java.library.path is not set properly by Windows jpackage app launcher
On Wed, 11 Aug 2021 17:52:57 GMT, Alexey Semenyuk wrote: > Restart window app launcher to make changes made to `PATH` env variable > visible to JLI code in > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L347 Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5090
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Marked as reviewed by naoto (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Marked as reviewed by dfuchs (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Marked as reviewed by alanb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 18:41:04 GMT, Claes Redestad wrote: > Yes, while I don't know exactly which changes resolved JDK-6764325, it's > clear from the microbenchmarks added for #2102 that it's no longer an issue - > at least not in the mainline. Thanks for checking and for closing that issue. - PR: https://git.openjdk.java.net/jdk/pull/5063
RFR: 8271302: Regex Test Refresh
8271302: Regex Test Refresh - Commit messages: - Migrating regular expression tests to TestNG Changes: https://git.openjdk.java.net/jdk/pull/5092/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5092=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271302 Stats: 2226 lines in 3 files changed: 225 ins; 965 del; 1036 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves wrote: > 8271302: Regex Test Refresh This PR migrates all regular expression tests in the jdk/java/util/regex directory to use TestNG assertions and annotations. The assertions utilized for this refresh are shared in common with standard ones from JUnit5 should such a migration occur in the future. - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Wed, 11 Aug 2021 11:27:53 GMT, Alan Bateman wrote: > I've looked through the latest revision. Is there any way that we could drop > most of the changes to ChannelInputStream and focus on one or two specific > cases? I'm asking because there are several issues, inconsistencies, and it > is trying to cover many scenarios that aren't covered by the test. > If the original motivation was file -> file then it could be simplified down > to a FileChannel -> FileChannel transfer as the default provider uses file > channels. We could even push some support into FileChannelImpl so that it is > done while holding the position lock. I am a bit disappointed actually about that destructive answer at that late point in time, now that I worked for months on all the requested changes and tests. To prevent exactly this situation, I deliberately had the discussion started in JIRA only, and I deliberately had the original code being just a draft in the first place, and I deliberately did nearly *everything* I was asked to (including even the most irrelevant minor code style issues). And you come up with the request to drop the code **now**? Certainly we could reduce the PR to just file channels, but in fact, now that I spent all the time in the non-file-channels, I wonder why I shall throw away all that work and go just with file channels actually? What is not covered that was originally covered, and what is that lots of issues you talk about? Actually I cannot see the actual problem unless you name it. - PR: https://git.openjdk.java.net/jdk/pull/4263
RFR: 8272328: java.library.path is not set properly by Windows jpackage app launcher
Restart window app launcher to make changes made to `PATH` env variable visible to JLI code in https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L347 - Commit messages: - 8272328: java.library.path is not set properly by Windows jpackage app launcher Changes: https://git.openjdk.java.net/jdk/pull/5090/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5090=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272328 Stats: 75 lines in 4 files changed: 63 ins; 5 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5090.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5090/head:pull/5090 PR: https://git.openjdk.java.net/jdk/pull/5090
Re: RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment [v3]
On Wed, 11 Aug 2021 15:20:00 GMT, Alexey Semenyuk wrote: >> Added jtreg test to verify jpackage launcher adds app dir to >> `java.library.path` system property. >> Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. >> Moved test/jdk/tools/jpackage/apps/image/Hello.java to >> test/jdk/tools/jpackage/apps/Hello.java. > > Alexey Semenyuk has updated the pull request incrementally with one > additional commit since the last revision: > > Removed unused output to file in the new PrintEnv test app. These changes will be pushed after https://bugs.openjdk.java.net/browse/JDK-8272328 is fixed. - PR: https://git.openjdk.java.net/jdk/pull/5056
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking
On Wed, 11 Aug 2021 14:26:32 GMT, Claes Redestad wrote: > In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. LGTM. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
+1
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment [v3]
> Added jtreg test to verify jpackage launcher adds app dir to > `java.library.path` system property. > Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. > Moved test/jdk/tools/jpackage/apps/image/Hello.java to > test/jdk/tools/jpackage/apps/Hello.java. Alexey Semenyuk has updated the pull request incrementally with one additional commit since the last revision: Removed unused output to file in the new PrintEnv test app. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5056/files - new: https://git.openjdk.java.net/jdk/pull/5056/files/4c0891dd..27e82df3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5056=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5056=01-02 Stats: 13 lines in 2 files changed: 0 ins; 11 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5056/head:pull/5056 PR: https://git.openjdk.java.net/jdk/pull/5056
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking
On Wed, 11 Aug 2021 14:26:32 GMT, Claes Redestad wrote: > In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. This looks okay but surprised there wasn't a jtreg test that would have caught this before integration, maybe we should add a test as part of this change. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5086
RFR: 8271732: Regression in StringBuilder.charAt bounds checking
In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to avoid potentially getting two bounds checks in the UTF-16 case. Problem is this optimization cause a regression since `StringUTF16.charAt(..)` checks `index` against the length of the `value` array and not `count`. A correct fix that still maintain the possible performance advantage reported by #4738 is to reinstate the `checkIndex(value, count)` and call `StringUTF16.getChar` instead of `charAt`. - Commit messages: - 8271732: Regression in StringBuilder.charAt bounds checking Changes: https://git.openjdk.java.net/jdk/pull/5086/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5086=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271732 Stats: 24 lines in 2 files changed: 22 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5086.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5086/head:pull/5086 PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Mon, 9 Aug 2021 18:10:52 GMT, Alan Bateman wrote: > I think I fixed all requested changes. Anymore comments on this PR? I've looked through the latest revision. Is here any way that we could drop most of the changes to ChannelInputStream and focus on one or two specific cases? I'm asking because there are several issues, inconsistencies, and it is trying to cover many scenarios that aren't covered by the test. If the original motivation was file -> file then it could be simplified down to a FileChannel -> FileChannel transfer as the default provider uses file channels. We could even push some support into FileChannelImpl so that it is done while holding the position lock. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used [v2]
On Tue, 10 Aug 2021 08:59:59 GMT, Per Liden wrote: >> While fixing JDK-8270626 it was discovered that the C2 intrinsic for >> `Reference.refersTo()` is not used as often as one would think. Instead C2 >> often prefers using the native implementation, which is much slower which >> defeats the purpose of having an intrinsic in the first place. >> >> The problem arise from having `refersTo0()` be virtual, native and >> @IntrinsicCandidate. This can be worked around by introducing an virtual >> method layer and so that `refersTo0()` can be made `final`. That's what this >> patch does, by adding a virtual `refersToImpl()` which in turn calls the now >> final `refersTo0()`. >> >> It should be noted that `Object.clone()` and `Object.hashCode()` are also >> virtual, native and @IntrinsicCandidate and these methods get special >> treatment by C2 to get the intrinsic generated more optimally. We might want >> to do a similar optimization for `Reference.refersTo0()`. In that case, it >> is suggested that such an improvement could come later and be handled as a >> separate enhancement, and @kimbarrett has already filed JDK-8272140 to track >> that. >> >> Testing: >> I found this hard to test without instrumenting Hotspot to tell me that the >> intrinsic was called instead of the native method. So, for that reason >> testing was ad-hoc, using an instrumented Hotspot in combination with the >> test >> (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) >> that initially uncovered the problem. See JDK-8270626 for additional >> information. > > Per Liden has updated the pull request incrementally with one additional > commit since the last revision: > > Private implies final Thanks all for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/5052
Integrated: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden wrote: > While fixing JDK-8270626 it was discovered that the C2 intrinsic for > `Reference.refersTo()` is not used as often as one would think. Instead C2 > often prefers using the native implementation, which is much slower which > defeats the purpose of having an intrinsic in the first place. > > The problem arise from having `refersTo0()` be virtual, native and > @IntrinsicCandidate. This can be worked around by introducing an virtual > method layer and so that `refersTo0()` can be made `final`. That's what this > patch does, by adding a virtual `refersToImpl()` which in turn calls the now > final `refersTo0()`. > > It should be noted that `Object.clone()` and `Object.hashCode()` are also > virtual, native and @IntrinsicCandidate and these methods get special > treatment by C2 to get the intrinsic generated more optimally. We might want > to do a similar optimization for `Reference.refersTo0()`. In that case, it is > suggested that such an improvement could come later and be handled as a > separate enhancement, and @kimbarrett has already filed JDK-8272140 to track > that. > > Testing: > I found this hard to test without instrumenting Hotspot to tell me that the > intrinsic was called instead of the native method. So, for that reason > testing was ad-hoc, using an instrumented Hotspot in combination with the > test > (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) > that initially uncovered the problem. See JDK-8270626 for additional > information. This pull request has now been integrated. Changeset: 3f723ca4 Author:Per Liden URL: https://git.openjdk.java.net/jdk/commit/3f723ca4577b9cffeb6153ee386edd75f1dfb1c6 Stats: 14 lines in 2 files changed: 11 ins; 0 del; 3 mod 8271862: C2 intrinsic for Reference.refersTo() is often not used Reviewed-by: kbarrett, mchung - PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
>From my pure developer's perspective the proposed changes look good. If the
>performance concerns are removed I'd say it looks good to me.
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Withdrawn: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Wed, 4 Aug 2021 12:22:24 GMT, Thomas Stuefe wrote: > We should not leak the handle to the jimage file (lib/modules) to childs. > > JDK-8194734 did solve this for windows. We should use FD_CLOEXEC on Posix as > well. > > Note that this only poses a problem when a child process is spawned off not > via `Runtime.exec` but via another route since the spawnhelper closes all > file descriptors. > > --- > test: > > manually verified that lib/modules now has the FD_CLOEXEC flag set. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4991
Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Wed, 4 Aug 2021 12:22:24 GMT, Thomas Stuefe wrote: > We should not leak the handle to the jimage file (lib/modules) to childs. > > JDK-8194734 did solve this for windows. We should use FD_CLOEXEC on Posix as > well. > > Note that this only poses a problem when a child process is spawned off not > via `Runtime.exec` but via another route since the spawnhelper closes all > file descriptors. > > --- > test: > > manually verified that lib/modules now has the FD_CLOEXEC flag set. I withdraw the PR and close the issue as won't fix since the issue is very unlikely to cause real problems (only in the event of someone raw-forking, bypassing Runtime.exec()). I'll find a narrower solution to simplify the test. - PR: https://git.openjdk.java.net/jdk/pull/4991
