Re: RFR: 8272805: Avoid looking up standard charsets
On Sun, 22 Aug 2021 02:53:44 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>
> 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.
>
> This change includes:
> * demo/utils
> * jdk.xx packages
> * Some places were missed in the previous changes. I have found it by
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop
> tests.
>
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>
> Code excluded in this fix: the Xerces library(should be fixed upstream),
> J2DBench(should be compatible to 1.4), some code in the network(the change
> there are not straightforward, will do it later).
>
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
The security related change looks fine to me.
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5210
Re: RFR: 8272805: Avoid looking up standard charsets
On Sun, 22 Aug 2021 02:53:44 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>
> 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.
>
> This change includes:
> * demo/utils
> * jdk.xx packages
> * Some places were missed in the previous changes. I have found it by
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop
> tests.
>
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>
> Code excluded in this fix: the Xerces library(should be fixed upstream),
> J2DBench(should be compatible to 1.4), some code in the network(the change
> there are not straightforward, will do it later).
>
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java line 342:
> 340:
> 341: try {
> 342: for (String line : Files.readAllLines(statusPath, UTF_8)) {
The 1-arg readAllLines is specified to use UTF-8 so you can drop the second
parameter here if you want.
-
PR: https://git.openjdk.java.net/jdk/pull/5210
Re: RFR: 8272805: Avoid looking up standard charsets
On Sun, 22 Aug 2021 02:53:44 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>
> 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.
>
> This change includes:
> * demo/utils
> * jdk.xx packages
> * Some places were missed in the previous changes. I have found it by
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop
> tests.
>
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>
> Code excluded in this fix: the Xerces library(should be fixed upstream),
> J2DBench(should be compatible to 1.4), some code in the network(the change
> there are not straightforward, will do it later).
>
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
I think it's worth to update _static_ initializer in
`sun.datatransfer.DataFlavorUtil.CharsetComparator` too.
-
PR: https://git.openjdk.java.net/jdk/pull/5210
Re: RFR: 8272805: Avoid looking up standard charsets [v2]
> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>
> 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.
>
> This change includes:
> * demo/utils
> * jdk.xx packages
> * Some places were missed in the previous changes. I have found it by
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop
> tests.
>
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>
> Code excluded in this fix: the Xerces library(should be fixed upstream),
> J2DBench(should be compatible to 1.4), some code in the network(the change
> there are not straightforward, will do it later).
>
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Sergey Bylokhov 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 14 additional commits
since the last revision:
- Update the usage of Files.readAllLines()
- Update datatransfer
- Merge branch 'master' into standard-encodings-in-non-public-modules
- Merge branch 'master' into standard-encodings-in-non-public-modules
- Fix related imports
- Merge branch 'master' into standard-encodings-in-non-public-modules
- Cleanup UnsupportedEncodingException
- Update PacketStream.java
- Rollback TextTests, should be compatible with jdk1.4
- Rollback TextRenderTests, should be compatible with jdk1.4
- ... and 4 more: https://git.openjdk.java.net/jdk/compare/37357100...e7127644
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5210/files
- new: https://git.openjdk.java.net/jdk/pull/5210/files/2d9c80b8..e7127644
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5210&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5210&range=00-01
Stats: 3598 lines in 210 files changed: 2055 ins; 1115 del; 428 mod
Patch: https://git.openjdk.java.net/jdk/pull/5210.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5210/head:pull/5210
PR: https://git.openjdk.java.net/jdk/pull/5210
Re: RFR: 8272805: Avoid looking up standard charsets [v2]
On Sun, 22 Aug 2021 23:02:06 GMT, Sergey Bylokhov wrote:
>> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>>
>> 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.
>>
>> This change includes:
>> * demo/utils
>> * jdk.xx packages
>> * Some places were missed in the previous changes. I have found it by
>> tracing the calls to the Charset.forName() by executing tier1,2,3 and
>> desktop tests.
>>
>> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>>
>> Code excluded in this fix: the Xerces library(should be fixed upstream),
>> J2DBench(should be compatible to 1.4), some code in the network(the change
>> there are not straightforward, will do it later).
>>
>> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
>
> Sergey Bylokhov 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 14 additional
> commits since the last revision:
>
> - Update the usage of Files.readAllLines()
> - Update datatransfer
> - Merge branch 'master' into standard-encodings-in-non-public-modules
> - Merge branch 'master' into standard-encodings-in-non-public-modules
> - Fix related imports
> - Merge branch 'master' into standard-encodings-in-non-public-modules
> - Cleanup UnsupportedEncodingException
> - Update PacketStream.java
> - Rollback TextTests, should be compatible with jdk1.4
> - Rollback TextRenderTests, should be compatible with jdk1.4
> - ... and 4 more:
> https://git.openjdk.java.net/jdk/compare/c262b06f...e7127644
Looks good.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5210
