Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
On Fri, 13 Jun 2025 18:10:03 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/java/math/BigDecimal.java line 1383:
>>
>>> 1381: * the result usually contains too many trailing digits compared
>>> 1382: * to the precision of a {@code float}.
>>> 1383: * Consider using {@code new BigDecimal(Float.toString(v))}
>>> instead.
>>
>> Perhaps move "Consider using" to the previous line; otherwise, looks fine.
>
> I usually start a sentence on a new line because that generates less noise
> when diffing in the future.
> The HTML renders it in the same paragraph as the preceding text.
Makes sense.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25805#discussion_r2145854316
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
On Fri, 13 Jun 2025 15:21:38 GMT, Raffaello Giulietti wrote: > Documenting a suggestion for `float` arguments. Marked as reviewed by bpb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25805#pullrequestreview-2925970196
RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
Documenting a suggestion for `float` arguments. - Commit messages: - 8358804: Improve the API Note of BigDecimal.valueOf(double) Changes: https://git.openjdk.org/jdk/pull/25805/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25805&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8358804 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/25805.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25805/head:pull/25805 PR: https://git.openjdk.org/jdk/pull/25805
Re: RFR: 8359123: Misleading examples in jmod man page
On Thu, 12 Jun 2025 07:57:20 GMT, Ana Maria Mihalceanu wrote: > Please review my PR. This PR includes the following: > > - [x] Fix a small typo in a word and copyright. > - [x] Enhance description for `--target-platform`. > - [x] Rearrange `jmod create` example from basic to complex. Changes requested by iris (Reviewer). src/jdk.jlink/share/man/jmod.md line 183: > 181: `--target-platform` *platform* > 182: : Specifies the target platform. The value is a string that identifies > 183: the platform this module is intended for, typically in the form > `-`. Consider: "The value is a string identifying the module's intended platform, typically of the form `-`. src/jdk.jlink/share/man/jmod.md line 220: > 218: deprecated, deprecated-for-removal, or incubating. > 219: > 220: ## jmod Create Example "Example" -> "Examples" src/jdk.jlink/share/man/jmod.md line 222: > 220: ## jmod Create Example > 221: > 222: The basic manner to create a JMOD file is by including only compiled > classes: I"m not sure what this is supposed to describe, perhaps something like this is more clear: "Create a JMOD file containing only compiled classes:". Whatever you choose should be parallel to the descriptions of the other create examples. (My later comments are parallel to my suggestion for this line.) src/jdk.jlink/share/man/jmod.md line 228: > 226: ``` > 227: > 228: To ensure reproducible artifacts, the following command creates Is it necessary to provide a reason to use this option rather than just describe what the example does? For parallelization, I'd retain the text in old line 231. src/jdk.jlink/share/man/jmod.md line 237: > 235: > 236: The command below creates a platform-specific JMOD file, bundling class > files, > 237: user-editable configuration files, header files, native commands and > libraries: Your description for the more complex command line should either list all or none of the options. If you chose the "none" approach, consider "Create a platform-specific JMOD file containing multiple artifacts." If you chose the "all" approach, you'll need to add references to the options on line 242, e.g. "Create a platform-specific JMOD file containing class files, user-editable configuration files, header files, ... ". - PR Review: https://git.openjdk.org/jdk/pull/25772#pullrequestreview-292112 PR Review Comment: https://git.openjdk.org/jdk/pull/25772#discussion_r2145550424 PR Review Comment: https://git.openjdk.org/jdk/pull/25772#discussion_r2145564293 PR Review Comment: https://git.openjdk.org/jdk/pull/25772#discussion_r2145578809 PR Review Comment: https://git.openjdk.org/jdk/pull/25772#discussion_r2145585506 PR Review Comment: https://git.openjdk.org/jdk/pull/25772#discussion_r2145676273
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v5]
On Fri, 13 Jun 2025 17:47:16 GMT, Justin Lu wrote:
>> Please review this PR which finishes Applet removal for the test:
>> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>>
>> `testclasses.jar` is updated such that the two classes no longer extend
>> Applet.
>>
>>
>> $ javap fo\ o.class
>> public class fo o {
>> }
>> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
>> public class 手册 {
>> }
>>
>>
>> The bug description of
>> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
>> original `javap` output for those classes.
>>
>> Additionally, the security APIs that were marked for removal are also
>> removed from this test as well.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review - Convert to JUnit, correct comment typo, remove 'Infra'
> methods
Thank you for the changes Justin
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25703#pullrequestreview-2925755658
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
On Fri, 13 Jun 2025 17:39:38 GMT, Brian Burkhalter wrote:
>> Documenting a suggestion for `float` arguments.
>
> src/java.base/share/classes/java/math/BigDecimal.java line 1383:
>
>> 1381: * the result usually contains too many trailing digits compared
>> 1382: * to the precision of a {@code float}.
>> 1383: * Consider using {@code new BigDecimal(Float.toString(v))}
>> instead.
>
> Perhaps move "Consider using" to the previous line; otherwise, looks fine.
I usually start a sentence on a new line because that generates less noise when
diffing in the future.
The HTML renders it in the same paragraph as the preceding text.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25805#discussion_r2145721480
Re: RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
On Tue, 7 Jan 2025 10:39:18 GMT, Shaojin Wen wrote: > In PR #22928, UUID introduced long-based vectorized hexadecimal to string > conversion, which can also be used in Integer::toHexString and > Long::toHexString to eliminate table lookups. The benefit of eliminating > table lookups is that the performance is better when cache misses occur. Performance test figures show that using the vectorized method toHex can improve performance in most cases by eliminating table lookups. ## 1. Script git remote add wenshao g...@github.com:wenshao/jdk.git git fetch wenshao # baseline git checkout 7731f4df6ba12b4e38f17f87bd42b1da6dc68f95 make test TEST="micro:java.lang.Integers.toHexString" make test TEST="micro:java.lang.Longs.toHexString" # current git checkout 6f491cedc235ab81b479620da3eb71385fc8 make test TEST="micro:java.lang.Integers.toHexString" make test TEST="micro:java.lang.Longs.toHexString" ## 2. aliyun_ecs_c8a_x64 (CPU AMD EPYC™ Genoa) -# 7731f4df6ba12b4e38f17f87bd42b1da6dc68f95 -Benchmark (size) Mode Cnt Score Error Units -Longs.toHexStringBig 500 avgt 15 5.984 ± 0.009 us/op -Longs.toHexStringSmall500 avgt 15 3.989 ± 0.036 us/op -Integers.toHexStringBig 500 avgt 15 4.473 ± 0.029 us/op -Integers.toHexStringSmall 500 avgt 15 4.166 ± 0.120 us/op -Integers.toHexStringTiny 500 avgt 15 3.394 ± 0.014 us/op +# 6f491cedc235ab81b479620da3eb71385fc8 +Benchmark (size) Mode Cnt Score Error Units +Longs.toHexStringBig 500 avgt 15 4.622 ± 0.011 us/op +29% +Longs.toHexStringSmall500 avgt 15 3.957 ± 0.035 us/op +0.8% +Integers.toHexStringBig 500 avgt 15 3.985 ± 0.019 us/op +12.24% +Integers.toHexStringSmall 500 avgt 15 3.617 ± 0.020 us/op +15.17% +Integers.toHexStringTiny 500 avgt 15 3.033 ± 0.015 us/op +11.90% ## 3. aliyun_ecs_c8i_x64 (CPU Intel®Xeon®Emerald Rapids) -# 7731f4df6ba12b4e38f17f87bd42b1da6dc68f95 -Benchmark (size) Mode Cnt Score Error Units -Longs.toHexStringBig 500 avgt 15 5.685 ± 0.019 us/op -Longs.toHexStringSmall500 avgt 15 3.914 ± 0.009 us/op -Integers.toHexStringBig 500 avgt 15 4.262 ± 0.025 us/op -Integers.toHexStringSmall 500 avgt 15 4.049 ± 0.012 us/op -Integers.toHexStringTiny 500 avgt 15 3.323 ± 0.015 us/op +# 6f491cedc235ab81b479620da3eb71385fc8 +Benchmark (size) Mode Cnt Score Error Units +Longs.toHexStringBig 500 avgt 15 4.791 ± 0.005 us/op +Longs.toHexStringSmall500 avgt 15 3.994 ± 0.022 us/op +Integers.toHexStringBig 500 avgt 15 4.184 ± 0.034 us/op +Integers.toHexStringSmall 500 avgt 15 3.771 ± 0.019 us/op +Integers.toHexStringTiny 500 avgt 15 3.133 ± 0.021 us/op ## 4. aliyun_ecs_c8y_aarch64 (CPU Aliyun Yitian 710) -# 7731f4df6ba12b4e38f17f87bd42b1da6dc68f95 -Benchmark (size) Mode Cnt Score Error Units -Longs.toHexStringBig 500 avgt 15 8.319 ± 0.139 us/op -Longs.toHexStringSmall500 avgt 15 4.985 ± 0.028 us/op -Integers.toHexStringBig 500 avgt 15 5.489 ± 0.041 us/op -Integers.toHexStringSmall 500 avgt 15 5.028 ± 0.033 us/op -Integers.toHexStringTiny 500 avgt 15 3.921 ± 0.023 us/op +# 6f491cedc235ab81b479620da3eb71385fc8 +Benchmark (size) Mode Cnt Score Error Units +Longs.toHexStringBig 500 avgt 15 5.346 ± 0.033 us/op +Longs.toHexStringSmall500 avgt 15 4.383 ± 0.027 us/op +Integers.toHexStringBig 500 avgt 15 4.821 ± 0.052 us/op +Integers.toHexStringSmall 500 avgt 15 4.428 ± 0.036 us/op +Integers.toHexStringTiny 500 avgt 15 3.800 ± 0.046 us/op ## 5. MacBook M1 Pro (aarch64) -# 7731f4df6ba12b4e38f17f87bd42b1da6dc68f95 -Benchmark (size) Mode Cnt Score Error Units -Longs.toHexStringBig 500 avgt 15 6.878 ± 0.230 us/op -Longs.toHexStringSmall500 avgt 15 4.975 ± 0.381 us/op -Integers.toHexStringBig 500 avgt 15 11.646 ± 2.699 us/op -Integers.toHexStringSmall 500 avgt 15 5.040 ± 0.319 us/op -Integers.toHexStringTiny 500 avgt 15 2.717 ± 0.023 us/op +# 6f491cedc235ab81b479620da3eb71385fc8 +Benchmark (size) Mode Cnt Score Error Units +Longs.toHexStringBig 500 avgt 15 4.007 ± 0.023 us/op +Longs.toHexStringSmall500 avgt 15 3.058 ± 0.024 us/op +Integers.toHexStringBig 500 avgt 15 3.399 ± 0.017 us/op +Integers.toHexStringSmall 500 avgt 15 3.163 ± 0.026 us/op +Integers.toHexStringTiny 500 avgt 15 2.909 ± 0.026 us/op - PR Comment: https://git.openjdk.org/jdk/pull/22942#issuecomment-2970383406
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
On Fri, 13 Jun 2025 15:21:38 GMT, Raffaello Giulietti
wrote:
> Documenting a suggestion for `float` arguments.
src/java.base/share/classes/java/math/BigDecimal.java line 1383:
> 1381: * the result usually contains too many trailing digits compared
> 1382: * to the precision of a {@code float}.
> 1383: * Consider using {@code new BigDecimal(Float.toString(v))} instead.
Perhaps move "Consider using" to the previous line; otherwise, looks fine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25805#discussion_r2145650829
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v4]
On Thu, 12 Jun 2025 21:42:26 GMT, Lance Andersen wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address offline review -> comments for maintainers, simplify exc and >> JAR_PATH > > test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java line 136: > >> 134: return defineClass(name, b, 0, b.length, >> codesource); >> 135: } >> 136: // protocol/host/port mismatch, fail with RuntimeExc > > Typo RuntimeExc? https://github.com/openjdk/jdk/pull/25703/commits/02c76c749015fc7d59992036625363d16773595f corrects the comment typo and also makes a conversion of this test to JUnit. - PR Review Comment: https://git.openjdk.org/jdk/pull/25703#discussion_r2145674512
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double) [v2]
> Documenting a suggestion for `float` arguments. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Suggestion by reviewer. - Changes: - all: https://git.openjdk.org/jdk/pull/25805/files - new: https://git.openjdk.org/jdk/pull/25805/files/9a5ff7c1..36b9a01e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25805&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25805&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/25805.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25805/head:pull/25805 PR: https://git.openjdk.org/jdk/pull/25805
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v5]
On Fri, 13 Jun 2025 17:47:16 GMT, Justin Lu wrote:
>> Please review this PR which finishes Applet removal for the test:
>> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>>
>> `testclasses.jar` is updated such that the two classes no longer extend
>> Applet.
>>
>>
>> $ javap fo\ o.class
>> public class fo o {
>> }
>> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
>> public class 手册 {
>> }
>>
>>
>> The bug description of
>> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
>> original `javap` output for those classes.
>>
>> Additionally, the security APIs that were marked for removal are also
>> removed from this test as well.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review - Convert to JUnit, correct comment typo, remove 'Infra'
> methods
test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java line 57:
> 55: static void setup() throws IOException {
> 56: var bytes = ClassFile.of().build(ClassDesc.of("fo o"), _ -> {});
> 57: try (JarOutputStream jos = new JarOutputStream(new
> FileOutputStream(JAR_PATH.toFile( {
Suggestion: The original test used testclasses.jar to provide two class files
with invalid names packaged in a jar. Since you now dynamically construct the
class files, there is no need to write them to disk as a jar. Why not have a
Maphttps://git.openjdk.org/jdk/pull/25703#discussion_r2146081990
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
On Fri, 13 Jun 2025 18:39:14 GMT, Brian Burkhalter wrote: >> I usually start a sentence on a new line because that generates less noise >> when diffing in the future. >> The HTML renders it in the same paragraph as the preceding text. > > Makes sense. > I usually start a sentence on a new line because that generates less noise > when diffing in the future. The HTML renders it in the same paragraph as the > preceding text. I use that authoring convention too; leads to clearer diffs on any future edits. - PR Review Comment: https://git.openjdk.org/jdk/pull/25805#discussion_r2146089711
Re: RFR: 8358880: Performance of parsing with DecimalFormat can be improved [v3]
On Thu, 12 Jun 2025 22:52:21 GMT, Justin Lu wrote: >> Unfortunately some check is required (a test fails), but I now realize what >> I had was wrong. The issue is that on line 1084 >> (https://github.com/openjdk/jdk/pull/25644/files#diff-79e6fd549b5ec5e7f49658581beddcb07fcbb0c09ae8e1117c385b66514da6d2R1084)) >> exp can overflow and become positive again. I've updated the check to avoid >> the overflow. > > Ah got it, I see your point. We would have goten underflow in > `ASCIIToBinaryConverter.doubleValue()` for some extreme cases without a > check. > > Is there a specific example you have that requires the switch to the newer > check? Adding a comment along those lines might be helpful. Actually, I > thought DigitList caps `decimalAt` to Integer.MIN/MAX, so then the first > check you had would have been fine. (Maybe I am missing something?) I don't have a specific example, so I've reverted to my original check. I'm a bit unsettled by the check for an extreme value later in `doubleValue()` comparing against `MIN_DECIMAL_EXPONENT - 1` - PR Review Comment: https://git.openjdk.org/jdk/pull/25644#discussion_r2145942325
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double) [v2]
On Fri, 13 Jun 2025 21:46:42 GMT, Raffaello Giulietti wrote: >> Documenting a suggestion for `float` arguments. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Suggestion by reviewer. Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25805#pullrequestreview-2926804580
Re: RFR: 8358880: Performance of parsing with DecimalFormat can be improved [v5]
> This PR replaces construction of intermediate strings to be parsed with more > direct manipulation of numbers. It also has a more streamlined mechanism of > handling `Long.MIN_VALUE` when parsing longs by using `Long.parseUnsignedLong` > > As a small side-effect it also eliminates the use of a cached StringBuilder > in DigitList. > > Testing: > - GHA > - Local run of tier 2 and jtreg:jdk/java/text > - New benchmark: DecimalFormatParseBench Johannes Graham has updated the pull request incrementally with one additional commit since the last revision: Revert "fix overflow check" This reverts commit 6a072873ffa87d1191b42053b9f5d955f0119057. - Changes: - all: https://git.openjdk.org/jdk/pull/25644/files - new: https://git.openjdk.org/jdk/pull/25644/files/6a072873..c87a3ded Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25644&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25644&range=03-04 Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/25644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25644/head:pull/25644 PR: https://git.openjdk.org/jdk/pull/25644
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double) [v2]
On Fri, 13 Jun 2025 21:46:42 GMT, Raffaello Giulietti wrote: >> Documenting a suggestion for `float` arguments. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Suggestion by reviewer. Marked as reviewed by bpb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25805#pullrequestreview-2926737056
Re: RFR: 8358804: Improve the API Note of BigDecimal.valueOf(double)
On Fri, 13 Jun 2025 15:21:38 GMT, Raffaello Giulietti
wrote:
> Documenting a suggestion for `float` arguments.
src/java.base/share/classes/java/math/BigDecimal.java line 1380:
> 1378: * Double#toString(double)}.
> 1379: *
> 1380: * While a {@code float} argument {@code v} can be passed to this
> method,
I recommend a slightly different wording; suggestion:
"...the result often contains many more trailing digits than the precision of a
`float`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25805#discussion_r2146108818
Re: RFR: 8359225: Remove unused test/jdk/javax/script/MyContext.java
On Wed, 11 Jun 2025 11:22:57 GMT, Volkan Yazici wrote: > Both `javax/script/PluggableContextTest.java` and its companion > `test/jdk/javax/script/MyContext.java` were added in > [JDK-6398614](https://bugs.openjdk.org/browse/JDK-6398614). > [JDK-8246113](https://bugs.openjdk.org/browse/JDK-8246113) removed > `PluggableContextTest`, yet forgot `MyContext`, and rendered it redundant. > Remove `MyContext` too. LGTM - Marked as reviewed by sundar (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25748#pullrequestreview-2923915788
Re: RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
On Tue, 7 Jan 2025 10:39:18 GMT, Shaojin Wen wrote: > In PR #22928, UUID introduced long-based vectorized hexadecimal to string > conversion, which can also be used in Integer::toHexString and > Long::toHexString to eliminate table lookups. The benefit of eliminating > table lookups is that the performance is better when cache misses occur. The testing data from both aarch64 and x64 architectures indicates a performance improvement of 10% to 20%. However, under the MacBook M1 Pro environment, the performance enhancement for the Integer.toHexString scenario has reached 100%. ## 1. Script git remote add wenshao g...@github.com:wenshao/jdk.git git fetch wenshao # baseline 91db7c0877a git checkout 91db7c0877a68ad171da2b4501280fc24630ae83 make test TEST="micro:java.lang.Integers.toHexString" make test TEST="micro:java.lang.Longs.toHexString" # current 1788d09787c git checkout 1788d09787cadfe6ec23b9b10bef87a2cdc029a3 make test TEST="micro:java.lang.Integers.toHexString" make test TEST="micro:java.lang.Longs.toHexString" ## 2. aliyun_ecs_c8a_x64 (CPU AMD EPYC™ Genoa) -Benchmark (size) Mode Cnt Score Error Units (baseline 91db7c0877a) -Integers.toHexString 500 avgt 15 4.855 ± 0.058 us/op -Longs.toHexString500 avgt 15 6.098 ± 0.034 us/op +Benchmark (size) Mode Cnt Score Error Units (current 1788d09787c) +Integers.toHexString 500 avgt 15 4.105 ± 0.010 us/op +18.27% +Longs.toHexString500 avgt 15 4.682 ± 0.116 us/op +30.24% ## 3. aliyun_ecs_c8i_x64 (CPU Intel®Xeon®Emerald Rapids) -Benchmark (size) Mode Cnt Score Error Units -Integers.toHexString 500 avgt 15 5.158 ± 0.025 us/op -Longs.toHexString500 avgt 15 6.072 ± 0.020 us/op +Benchmark (size) Mode Cnt Score Error Units +Integers.toHexString 500 avgt 15 4.691 ± 0.024 us/op +9.95% +Longs.toHexString500 avgt 15 4.947 ± 0.024 us/op +22.74% ## 4. aliyun_ecs_c8y_aarch64 (CPU Aliyun Yitian 710) -Benchmark (size) Mode Cnt Score Error Units -Integers.toHexString 500 avgt 15 5.880 ± 0.017 us/op -Longs.toHexString500 avgt 15 7.183 ± 0.013 us/op +Benchmark (size) Mode Cnt Score Error Units +Integers.toHexString 500 avgt 15 5.282 ± 0.012 us/op +11.32% +Longs.toHexString500 avgt 15 5.530 ± 0.013 us/op +29.89% ## 5. MacBook M1 Pro (aarch64) -Benchmark (size) Mode Cnt Score Error Units (baseline 91db7c0877a) -Integers.toHexString 500 avgt 15 10.519 ? 1.573 us/op -Longs.toHexString500 avgt 15 5.754 ? 0.264 us/op +Benchmark (size) Mode Cnt Score Error Units (current 1788d09787c) +Integers.toHexString 500 avgt 15 5.057 ? 0.015 us/op +108.00% +Longs.toHexString500 avgt 15 5.147 ? 0.095 us/op +11.79% Because this algorithm underperforms compared to the original version when handling smaller numbers, I have marked this PR as draft. Additionally, this algorithm is used in another PR #22928 [Speed up UUID::toString](https://github.com/openjdk/jdk/pull/22928) , and it still experiences performance degradation with Long.expand on older CPU architectures. // Method 1: i = Long.reverseBytes(Long.expand(i, 0x0F0F_0F0F_0F0F_0F0FL)); // Method 2: i = ((i & 0xF000L) >> 28) | ((i & 0xF00L) >> 16) | ((i & 0xF0L) >> 4) | ((i & 0xFL) << 8) | ((i & 0xF000L) << 20) | ((i & 0xF00L) << 32) | ((i & 0xF0L) << 44) | ((i & 0xFL) << 56); Note: Using Long.reverseBytes + Long.expand is faster on x64 and ARMv9. However, on AArch64 with ARMv8, it will be slower compared to the manual unrolling shown in Method 2. ARMv8 includes Apple M1/M2, AWS Graviton 3; ARMv9.0 includes Apple M3/M4, Aliyun Yitian 710. I haven't tested this on older x64 CPUs, like the AMD ZEN1, but it's possible that they experience the same issue. - PR Comment: https://git.openjdk.org/jdk/pull/22942#issuecomment-2576197320 PR Comment: https://git.openjdk.org/jdk/pull/22942#issuecomment-2578863538
Re: RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
On Tue, 7 Jan 2025 10:39:18 GMT, Shaojin Wen wrote: > In PR #22928, UUID introduced long-based vectorized hexadecimal to string > conversion, which can also be used in Integer::toHexString and > Long::toHexString to eliminate table lookups. The benefit of eliminating > table lookups is that the performance is better when cache misses occur. Running it on my machine (Ryzen 9 3900X): baseline (91db7c0877a68ad171da2b4501280fc24630ae83): Integers.toHexString 500 avgt 15 5.717 ± 0.274 us/op Longs.toHexString500 avgt 15 6.851 ± 0.214 us/op this change (1788d09787cadfe6ec23b9b10bef87a2cdc029a3): Integers.toHexString 500 avgt 15 21.334 ± 0.268 us/op Longs.toHexString500 avgt 15 38.907 ± 0.589 us/op I know that this processor has an extremely slow implementation of the `PDEP` instruction, but I'm kinda surprised you're seeing better results on aarch64. But I think Zen1 and Zen2 should considered here to avoid regressions on those architectures. - PR Comment: https://git.openjdk.org/jdk/pull/22942#issuecomment-2577153462
Re: RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
On Tue, 7 Jan 2025 10:39:18 GMT, Shaojin Wen wrote:
> In PR #22928, UUID introduced long-based vectorized hexadecimal to string
> conversion, which can also be used in Integer::toHexString and
> Long::toHexString to eliminate table lookups. The benefit of eliminating
> table lookups is that the performance is better when cache misses occur.
I think that without proper assembly analysis won't be easy to check why...
And yes, pdep is bad in old Ryzen @SirYwell :"(
It could be either a branch prediction problem too (perfnorm would help) if the
list of longs can produce small/big hex strings
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 199:
> 197:
> 198: /**
> 199: * Extract the least significant 8 bytes from the input integer i,
> convert each byte into its corresponding 2-digit
The least significant 4 bytes
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 204:
> 202: */
> 203: public static long hex8(long i) {
> 204: long x = Long.expand(i, 0x0F0F_0F0F_0F0F_0F0FL);
x86 should use pepd - but aarch64?
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 228:
> 226: return ((m << 1) + (m >> 1) - (m >> 4))
> 227: + 0x3030_3030_3030_3030L
> 228: + (x & 0x0F0F_0F0F_0F0F_0F0FL);
x is already expanded at 0x0F0F_0F0F_0F0F_0F0FL, why & it again?
Another thing: IDK how C2 does math here, but on the assembly it should be
straightforward to check if we have some register data dep while performing
these series of addition/subtraction.
Usually x86 is more affected by this since it has less register available
-
PR Comment: https://git.openjdk.org/jdk/pull/22942#issuecomment-2577178403
PR Review Comment: https://git.openjdk.org/jdk/pull/22942#discussion_r1906110618
PR Review Comment: https://git.openjdk.org/jdk/pull/22942#discussion_r1906104894
PR Review Comment: https://git.openjdk.org/jdk/pull/22942#discussion_r1906103700
Re: RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
On Tue, 7 Jan 2025 10:39:18 GMT, Shaojin Wen wrote:
> In PR #22928, UUID introduced long-based vectorized hexadecimal to string
> conversion, which can also be used in Integer::toHexString and
> Long::toHexString to eliminate table lookups. The benefit of eliminating
> table lookups is that the performance is better when cache misses occur.
src/java.base/share/classes/java/lang/Integer.java line 312:
> 310: * this string as a hexadecimal number to form and return a long
> value.
> 311: */
> 312: static long hex8(long i) {
I think we should move this to HexDigits.
src/java.base/share/classes/java/lang/Long.java line 325:
> 323: if (COMPACT_STRINGS) {
> 324: len -= 8;
> 325: Unsafe.getUnsafe().putLong(chars, ARRAY_BYTE_BASE_OFFSET
> + len, Long.reverseBytes(x));
We only need to reverse on small endian platforms right? We can use
putLongUnaligned which takes a boolean for big endian so conversion is
automatic.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22942#discussion_r1905774332
PR Review Comment: https://git.openjdk.org/jdk/pull/22942#discussion_r1905786538
RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
In PR #22928, UUID introduced long-based vectorized hexadecimal to string conversion, which can also be used in Integer::toHexString and Long::toHexString to eliminate table lookups. The benefit of eliminating table lookups is that the performance is better when cache misses occur. - Commit messages: - Merge remote-tracking branch 'upstream/master' into optim_to_hex_202501 - use right shift - use right shift - fix benchmark - Merge remote-tracking branch 'upstream/master' into optim_to_hex_202501 - Merge remote-tracking branch 'upstream/master' into optim_to_hex_202501 - Merge remote-tracking branch 'upstream/master' into optim_to_hex_202501 - reverseBytes - reverseBytes - public hex8 - ... and 9 more: https://git.openjdk.org/jdk/compare/e18277b4...6f491ced Changes: https://git.openjdk.org/jdk/pull/22942/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22942&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8359424 Stats: 262 lines in 6 files changed: 178 ins; 77 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/22942.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22942/head:pull/22942 PR: https://git.openjdk.org/jdk/pull/22942
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v4]
On Thu, 12 Jun 2025 18:05:04 GMT, Justin Lu wrote:
>> Please review this PR which finishes Applet removal for the test:
>> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>>
>> `testclasses.jar` is updated such that the two classes no longer extend
>> Applet.
>>
>>
>> $ javap fo\ o.class
>> public class fo o {
>> }
>> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
>> public class 手册 {
>> }
>>
>>
>> The bug description of
>> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
>> original `javap` output for those classes.
>>
>> Additionally, the security APIs that were marked for removal are also
>> removed from this test as well.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address offline review -> comments for maintainers, simplify exc and
> JAR_PATH
Thank you for the updates Justin.
Looks good overall. If you have to update this test again, it might be
worthwhile considering converting to a junit test
test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java line 136:
> 134: return defineClass(name, b, 0, b.length, codesource);
> 135: }
> 136: // protocol/host/port mismatch, fail with RuntimeExc
Typo RuntimeExc?
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25703#pullrequestreview-2922739662
PR Review Comment: https://git.openjdk.org/jdk/pull/25703#discussion_r2143711460
Re: RFR: 8359424: Eliminate table lookup in Integer/Long toHexString
On Tue, 7 Jan 2025 22:16:19 GMT, Francesco Nigro wrote:
>> In PR #22928, UUID introduced long-based vectorized hexadecimal to string
>> conversion, which can also be used in Integer::toHexString and
>> Long::toHexString to eliminate table lookups. The benefit of eliminating
>> table lookups is that the performance is better when cache misses occur.
>
> src/java.base/share/classes/jdk/internal/util/HexDigits.java line 204:
>
>> 202: */
>> 203: public static long hex8(long i) {
>> 204: long x = Long.expand(i, 0x0F0F_0F0F_0F0F_0F0FL);
>
> x86 should use pepd - but aarch64?
Seems there is no good way to do so on aarch; raw shifts and going through VPU
both seem to be slower.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22942#discussion_r1906141022
Integrated: 8359225: Remove unused test/jdk/javax/script/MyContext.java
On Wed, 11 Jun 2025 11:22:57 GMT, Volkan Yazici wrote: > Both `javax/script/PluggableContextTest.java` and its companion > `test/jdk/javax/script/MyContext.java` were added in > [JDK-6398614](https://bugs.openjdk.org/browse/JDK-6398614). > [JDK-8246113](https://bugs.openjdk.org/browse/JDK-8246113) removed > `PluggableContextTest`, yet forgot `MyContext`, and rendered it redundant. > Remove `MyContext` too. This pull request has now been integrated. Changeset: 3a188726 Author:Volkan Yazici URL: https://git.openjdk.org/jdk/commit/3a1887269b9cecf9dea68637f99b0b103baafbdb Stats: 222 lines in 1 file changed: 0 ins; 222 del; 0 mod 8359225: Remove unused test/jdk/javax/script/MyContext.java Reviewed-by: sundar - PR: https://git.openjdk.org/jdk/pull/25748
Re: RFR: 8359225: Remove unused test/jdk/javax/script/MyContext.java
On Fri, 13 Jun 2025 08:52:37 GMT, Athijegannathan Sundararajan wrote: >> Both `javax/script/PluggableContextTest.java` and its companion >> `test/jdk/javax/script/MyContext.java` were added in >> [JDK-6398614](https://bugs.openjdk.org/browse/JDK-6398614). >> [JDK-8246113](https://bugs.openjdk.org/browse/JDK-8246113) removed >> `PluggableContextTest`, yet forgot `MyContext`, and rendered it redundant. >> Remove `MyContext` too. > > LGTM @sundararajana, thanks for the review. For completeness, `tier1,2` results are attached to the ticket. - PR Comment: https://git.openjdk.org/jdk/pull/25748#issuecomment-2969655592
Re: RFR: 8357995: Use "stdin.encoding" for reading System.in with InputStreamReader/Scanner [core] [v7]
> Passes the `Charset` read from the `stdin.encoding` system property while > creating `InputStreamReader` or `Scanner` instances for `System.in`. > > `stdin.encoding` is a recently added property for Java 25 in > [JDK-8350703](https://bugs.openjdk.org/browse/JDK-8350703). Employing it > throughout the entire code base is addressed by the parent ticket > [JDK-8356893](https://bugs.openjdk.org/browse/JDK-8356893). JDK-8357995 this > PR is addressing is a sub-task of JDK-8356893 and is concerned with only > areas related to core libraries. Volkan Yazici has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Merge remote-tracking branch 'upstream/master' into stdinEnc-core - Revert superfluous test changes - Improve code style Co-authored-by: Andrey Turbanov - Fix missing `java.io.Reader` import in `Ktab` - Clean-up `MultiBreakpointsTarg` - Provide fallback for `stdin.encoding` - Revert changes to `Application` and `JavaChild` There stdin is connected to the parent process rather than the console. - Revert more superfluous changes - Fix code typo - Discard changes unrelated with core libraries - ... and 4 more: https://git.openjdk.org/jdk/compare/9aeacf2d...d36e0bd5 - Changes: https://git.openjdk.org/jdk/pull/25544/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25544&range=06 Stats: 130 lines in 11 files changed: 33 ins; 28 del; 69 mod Patch: https://git.openjdk.org/jdk/pull/25544.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25544/head:pull/25544 PR: https://git.openjdk.org/jdk/pull/25544
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v5]
> Please review this PR which finishes Applet removal for the test:
> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>
> `testclasses.jar` is updated such that the two classes no longer extend
> Applet.
>
>
> $ javap fo\ o.class
> public class fo o {
> }
> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
> public class 手册 {
> }
>
>
> The bug description of
> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
> original `javap` output for those classes.
>
> Additionally, the security APIs that were marked for removal are also removed
> from this test as well.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
Address review - Convert to JUnit, correct comment typo, remove 'Infra'
methods
-
Changes:
- all: https://git.openjdk.org/jdk/pull/25703/files
- new: https://git.openjdk.org/jdk/pull/25703/files/47f62aa2..02c76c74
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=25703&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=25703&range=03-04
Stats: 110 lines in 1 file changed: 22 ins; 61 del; 27 mod
Patch: https://git.openjdk.org/jdk/pull/25703.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/25703/head:pull/25703
PR: https://git.openjdk.org/jdk/pull/25703
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v5]
On Fri, 13 Jun 2025 17:47:16 GMT, Justin Lu wrote:
>> Please review this PR which finishes Applet removal for the test:
>> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>>
>> `testclasses.jar` is updated such that the two classes no longer extend
>> Applet.
>>
>>
>> $ javap fo\ o.class
>> public class fo o {
>> }
>> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
>> public class 手册 {
>> }
>>
>>
>> The bug description of
>> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
>> original `javap` output for those classes.
>>
>> Additionally, the security APIs that were marked for removal are also
>> removed from this test as well.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review - Convert to JUnit, correct comment typo, remove 'Infra'
> methods
test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java line 57:
> 55: static void setup() throws IOException {
> 56: var bytes = ClassFile.of().build(ClassDesc.of("fo o"), _ -> {});
> 57: try (JarOutputStream jos = new JarOutputStream(new
> FileOutputStream(JAR_PATH.toFile( {
Suggestion: The original test used testclasses.jar to provide two class files
with invalid names packaged in a jar. Since you now dynamically construct the
class files, there is no need to write them to disk as a jar. Why not have a
Maphttps://git.openjdk.org/jdk/pull/25703#discussion_r2145924359
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v2]
On Thu, 12 Jun 2025 15:29:55 GMT, Jaikiran Pai wrote: > I went back and looked at the JDK-5017871 issue through which the `fo > o.class` was being tested and from what I see in there, the current test > changes continue to test that issue. I think there might be better ways to > test that original use case and this test could be simplified a lot, but it's > certainly not for this PR. Maybe labelling this test with `@bug 4957669 5017871` was a mistake, test/jdk/java/net/Socket/IDNTest.java does address 4957669 and 5017871. - PR Comment: https://git.openjdk.org/jdk/pull/25703#issuecomment-2971687510
