Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 19:21:13 GMT, Jonathan Gibbons wrote: >> Please review a set of updates to clean up use of `/**` comments in the >> vicinity of declarations. >> >> There are various categories of update: >> >> * "Box comments" beginning with `/**` >> * Misplaced doc comments before package or import statements >> * Misplaced doc comments after the annotations for a declaration >> * Dangling `/**` comments relating to a group of declarations, separate from >> the doc comments for each of those declarations >> * Use of `/**` for the legal header at or near the top of the file >> >> In one case, two doc comments before a declaration were merged, which fixes >> a bug/omission in the documented serialized form. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Update > src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java > > Fix grammatical typo > > Co-authored-by: Alexey Ivanov Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18846#pullrequestreview-2016126206
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 19:21:13 GMT, Jonathan Gibbons wrote: >> Please review a set of updates to clean up use of `/**` comments in the >> vicinity of declarations. >> >> There are various categories of update: >> >> * "Box comments" beginning with `/**` >> * Misplaced doc comments before package or import statements >> * Misplaced doc comments after the annotations for a declaration >> * Dangling `/**` comments relating to a group of declarations, separate from >> the doc comments for each of those declarations >> * Use of `/**` for the legal header at or near the top of the file >> >> In one case, two doc comments before a declaration were merged, which fixes >> a bug/omission in the documented serialized form. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Update > src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java > > Fix grammatical typo > > Co-authored-by: Alexey Ivanov Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18846#pullrequestreview-2016121831
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Mon, 22 Apr 2024 19:49:44 GMT, Brian Burkhalter wrote: >> Prevent blocking due to a carrier thread not being released when >> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > Correct ID in test @bug tag Tiers 1-3 passed on the usual platforms. - PR Comment: https://git.openjdk.org/jdk/pull/18901#issuecomment-2071221654
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v23]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: add details on use of reference queues; swap reachability/memviz sections - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/91d4db48..27d0c249 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=22 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=21-22 Stats: 81 lines in 1 file changed: 41 ins; 33 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v17]
> Re-write the IndexOf code without the use of the pcmpestri instruction, only > using AVX2 instructions. This change accelerates String.IndexOf on average > 1.3x for AVX2. The benchmark numbers: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Move arrays_equals back to c2_MacroAssembler - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/8e0ce70a..1d141fde Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=16 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=15-16 Stats: 576 lines in 5 files changed: 288 ins; 282 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: 8319990: Update CLDR to Version 45.0
On Mon, 22 Apr 2024 18:44:33 GMT, Naoto Sato wrote: > Upgrading the CLDR to version 45.0. We now have versions specified in the > javadoc (`LocaleServiceProvider`), a corresponding CSR has also been drafted. LGTM; `LocaleServiceProvider` displays correct CLDR version (45), and Italian compact millions is fixed in regression tests - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18900#pullrequestreview-2015776529
Re: RFR: 8319990: Update CLDR to Version 45.0
On Mon, 22 Apr 2024 18:44:33 GMT, Naoto Sato wrote: > Upgrading the CLDR to version 45.0. We now have versions specified in the > javadoc (`LocaleServiceProvider`), a corresponding CSR has also been drafted. Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18900#pullrequestreview-2015768841
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
> Prevent blocking due to a carrier thread not being released when > `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: Correct ID in test @bug tag - Changes: - all: https://git.openjdk.org/jdk/pull/18901/files - new: https://git.openjdk.org/jdk/pull/18901/files/08bb9cb6..57e4917b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18901=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18901=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18901.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18901/head:pull/18901 PR: https://git.openjdk.org/jdk/pull/18901
RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier
Prevent blocking due to a carrier thread not being released when `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. - Commit messages: - 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier Changes: https://git.openjdk.org/jdk/pull/18901/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18901=00 Issue: https://bugs.openjdk.org/browse/JDK-8330748 Stats: 137 lines in 2 files changed: 134 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18901.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18901/head:pull/18901 PR: https://git.openjdk.org/jdk/pull/18901
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v3]
On Fri, 19 Apr 2024 19:39:09 GMT, Sonia Zaldana Calles
wrote:
>> Hi folks,
>>
>> This PR aims to fix
>> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581).
>>
>> I think the regression got introduced in
>> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458).
>>
>> In the issue linked above,
>> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226)
>> got removed to simplify launcher code.
>>
>> Previously, we used ```getMainType``` to do the appropriate main method
>> invocation in ```JavaMain```. However, we currently attempt to do all types
>> of main method invocations at the same time
>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623).
>>
>>
>> Note how all of these invocations clear the exception reported with
>> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390).
>>
>>
>> Therefore, if a legitimate exception comes up during one of these
>> invocations, it does not get reported.
>>
>> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking
>> forward to your suggestions.
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixing space formatting
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 908:
> 906:
> 907: private static boolean isStatic = false;
> 908: private static boolean noArgs = false;
Suggestion:
private static boolean isStaticMain = false;
private static boolean noArgMain = false;
src/java.base/share/native/libjli/java.c line 396:
> 394: int
> 395: invokeStaticMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
> mainArgs,
> 396: JavaVM *vm, int ret) {
As `CHECK_EXCEPTION_xxx_LEAVE` assumes to be used within JavaMain and it
changes the value of the local `ret` variable, it should call
`CHECK_EXCEPTION_RETURN_VALUE` and `NULL_CHECK_RETURN_VALUE` instead.
JavaMain should call `CHECK_EXCEPTION_LEAVE(1);` after this method returns to
print any exception and exit.
src/java.base/share/native/libjli/java.c line 399:
> 397: jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main",
> 398: "([Ljava/lang/String;)V");
> 399: CHECK_EXCEPTION_LEAVE(1);
Is this needed? `invokeStaticMainWithArgs` should only be called if the main
method is validated as a static main with args.
A sanity check `NULL_CHECK0(mainID)` may be adequate (to use existing macro and
so keeping the return value 0 to indicate error).
src/java.base/share/native/libjli/java.c line 409:
> 407: */
> 408: int
> 409: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
> mainArgs,
int
invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
mainArgs) {
jmethodID mainID = (*env)->GetMethodID(env, mainClass, "main",
"([Ljava/lang/String;)V");
NULL_CHECK0(mainID);
jmethodID constructor = (*env)->GetMethodID(env, mainClass, "",
"()V");
NULL_CHECK0(constructor);
jobject mainObject = (*env)->NewObject(env, mainClass, constructor);
CHECK_EXCEPTION_RETURN_VALUE(0);
NULL_CHECK0(mainObject);
(*env)->CallVoidMethod(env, mainObject, mainID, mainArgs);
return 1;
}
src/java.base/share/native/libjli/java.c line 431:
> 429: jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main",
> 430:"()V");
> 431: CHECK_EXCEPTION_LEAVE(1);
Same comment as `invokeStaticMainWithArgs`.
Suggestion:
NULL_CHECK0(mainID);
src/java.base/share/native/libjli/java.c line 441:
> 439: */
> 440: int
> 441: invokeInstanceMainWithoutArgs(JNIEnv *env, jclass mainClass,
See the suggestion for `invokeInstanceMainWithArgs`
src/java.base/share/native/libjli/java.c line 610:
> 608:
> 609:
> 610: jclass helperClass = GetLauncherHelperClass(env);
Follow this file convention, declare all local variables at the beginning of
this function.
src/java.base/share/native/libjli/java.c line 614:
> 612: (*env)->GetStaticFieldID(env, helperClass, "isStatic", "Z");
> 613: jboolean isStatic =
> 614: (*env)->GetStaticBooleanField(env, helperClass, isStaticField);
Check exception. Local variable declared at the beginning of the function.
Suggestion:
isStaticMainField = (*env)->GetStaticFieldID(env, helperClass,
"isStaticMain", "Z");
CHECK_EXCEPTION_NULL_LEAVE(isStaticMain);
isStaticMain = (*env)->GetStaticBooleanField(env, helperClass,
isStaticMainField);
src/java.base/share/native/libjli/java.c line 619:
> 617: (*env)->GetStaticFieldID(env, helperClass, "noArgs", "Z");
> 618: jboolean noArgs =
> 619:
RFR: 8319990: Update CLDR to Version 45.0
Upgrading the CLDR to version 45.0. We now have versions specified in the javadoc (`LocaleServiceProvider`), a corresponding CSR has also been drafted. - Commit messages: - .md files - release-45 - Fix tests for CLDR-17482 Fix million compact number issue in Italian - release-45-beta4 - release-v45-beta3 - Added CLDR 45 release info - release-45-beta1 - release-45-alpha3 - release-45-alpha2 - release-45-alpha0 - ... and 1 more: https://git.openjdk.org/jdk/compare/140f5671...90a1edcb Changes: https://git.openjdk.org/jdk/pull/18900/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18900=00 Issue: https://bugs.openjdk.org/browse/JDK-8319990 Stats: 7401 lines in 1075 files changed: 307 ins; 4640 del; 2454 mod Patch: https://git.openjdk.org/jdk/pull/18900.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18900/head:pull/18900 PR: https://git.openjdk.org/jdk/pull/18900
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
On Mon, 22 Apr 2024 12:45:53 GMT, Alan Bateman wrote: >> This change drops the adjustments to the virtual thread scheduler's target >> parallelism when virtual threads do file operations on files that are opened >> for buffered I/O. These operations are usually just too short to have any >> benefit and may have a negative benefit when reading/writing a small number >> of bytes. There is no change for read/write operations on files opened for >> direct I/O or when writing to files that are opened with options for >> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery >> Kuksenko is polishing benchmarks that includes this area, this is for a >> future PR. >> >> In addition, the blocker mechanism is updated to handle reentrancy (as can >> happen if debugging code is added to ForkJoinPool) and preemption when >> compensating (as can happen when substituting a heap buffer with a direct >> buffer in some I/O operations). This part is a pre-requisite to the changes >> to better support object monitor there are more places where preemption is >> possible and this quickly leads to unbalanced begin/end. >> >> The changes have been baking in loom repo for some time. > > Alan Bateman 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 five additional > commits since the last revision: > > - Merge > - Sync up from loom repo, update copyright headers > - Merge > - Merge > - Initial commit Looks fine. I think dropping the `transferTo` overloads for now is good. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18598#pullrequestreview-2015549163
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Mon, 22 Apr 2024 17:38:59 GMT, Jonathan Gibbons wrote: > The document [How to Write Doc Comments for the Javadoc > Tool](https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html) > is depressingly obsolete, as indicated by this text towards the end: I know. Yet there's nothing newer, is there? - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2070433346
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Fri, 19 Apr 2024 19:29:31 GMT, Alexey Ivanov wrote: > > We do not have an overall style guide. The conventional wisdom for editing > > any existing file is to follow the style in that file, if such a style can > > be discerned. > > That's what I do. > > I saw either style used in JDK. Yet I didn't check which style is more > common… to decide which style I should use when creating new files with > javadoc comments. [How to Write Doc Comments for the Javadoc > Tool](https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html) > doesn't have a blank line between the javadoc comment and class declaration; > nor do javadoc comments for fields and methods. The document [How to Write Doc Comments for the Javadoc Tool](https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html) is depressingly obsolete, as indicated by this text towards the end: >Footnotes > > [1] At Java Software, we use @version for the SCCS version. See "man > sccs-get" for details. The consensus seems to be the following: - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2070379608
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Fri, 19 Apr 2024 10:07:22 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove launcher executable path trace output It helped to understand the issue behind https://bugs.openjdk.org/browse/JDK-8329653 - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2070365643
Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]
On Mon, 22 Apr 2024 14:11:41 GMT, Claes Redestad wrote: >> This switch expression in `Locale::createLocale` is causing a somewhat large >> startup regression on my local system. Desugaring to if statements seem like >> the right thing to do while we investigate ways to further reduce overheads >> in `SwitchBootstraps`. >> >> These numbers are against a baseline which include #18865 and #18845, which >> already improved the situation: >> >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2040,500 ± 1,942 31,000 ± >> 2,673ms/op 1,31x (p = 0,000*) >> :.cycles 143254849,000 ± 3398321,355 102205427,650 ± >> 2192784,853 cycles 0,71x (p = 0,000*) >> :.instructions 307138448,850 ± 2095834,550 219415574,800 ± >> 376992,067 instructions 0,71x (p = 0,000*) >> :.taskclock 39,500 ± 1,942 22,500 ± >> 3,858 ms 0,57x (p = 0,000*) >> * = significant >> >> >> Comparing to a baseline without those recent improvements the overhead was >> almost the double: >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2050,000 ± 0,000 31,000 ± >> 2,673ms/op 1,61x (p = 0,000*) >> :.cycles 187047932,000 ± 3330400,381 102205427,650 ± >> 2192784,853 cycles 0,55x (p = 0,000*) >> :.instructions 408219060,350 ± 4031173,140 219415574,800 ± >> 376992,067 instructions 0,54x (p = 0,000*) >> :.taskclock 53,500 ± 4,249 22,500 ± >> 3,858 ms 0,42x (p = 0,000*) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove InternalError in favor of CCE Looks good. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2015399334
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Fri, 19 Apr 2024 10:07:22 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove launcher executable path trace output What is the use case for this tracing functionality? I recently was quite helped by it when debugging static java launching. And in that case, the more logging the better. But that sounds like a very special case. Is this something that end users are supposed to use to help solve problems? - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2070309758
Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]
On Mon, 22 Apr 2024 14:11:41 GMT, Claes Redestad wrote: >> This switch expression in `Locale::createLocale` is causing a somewhat large >> startup regression on my local system. Desugaring to if statements seem like >> the right thing to do while we investigate ways to further reduce overheads >> in `SwitchBootstraps`. >> >> These numbers are against a baseline which include #18865 and #18845, which >> already improved the situation: >> >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2040,500 ± 1,942 31,000 ± >> 2,673ms/op 1,31x (p = 0,000*) >> :.cycles 143254849,000 ± 3398321,355 102205427,650 ± >> 2192784,853 cycles 0,71x (p = 0,000*) >> :.instructions 307138448,850 ± 2095834,550 219415574,800 ± >> 376992,067 instructions 0,71x (p = 0,000*) >> :.taskclock 39,500 ± 1,942 22,500 ± >> 3,858 ms 0,57x (p = 0,000*) >> * = significant >> >> >> Comparing to a baseline without those recent improvements the overhead was >> almost the double: >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2050,000 ± 0,000 31,000 ± >> 2,673ms/op 1,61x (p = 0,000*) >> :.cycles 187047932,000 ± 3330400,381 102205427,650 ± >> 2192784,853 cycles 0,55x (p = 0,000*) >> :.instructions 408219060,350 ± 4031173,140 219415574,800 ± >> 376992,067 instructions 0,54x (p = 0,000*) >> :.taskclock 53,500 ± 4,249 22,500 ± >> 3,858 ms 0,42x (p = 0,000*) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove InternalError in favor of CCE Thanks for the fix, Claes! - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2015234211
Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]
On Mon, 22 Apr 2024 14:11:41 GMT, Claes Redestad wrote: >> This switch expression in `Locale::createLocale` is causing a somewhat large >> startup regression on my local system. Desugaring to if statements seem like >> the right thing to do while we investigate ways to further reduce overheads >> in `SwitchBootstraps`. >> >> These numbers are against a baseline which include #18865 and #18845, which >> already improved the situation: >> >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2040,500 ± 1,942 31,000 ± >> 2,673ms/op 1,31x (p = 0,000*) >> :.cycles 143254849,000 ± 3398321,355 102205427,650 ± >> 2192784,853 cycles 0,71x (p = 0,000*) >> :.instructions 307138448,850 ± 2095834,550 219415574,800 ± >> 376992,067 instructions 0,71x (p = 0,000*) >> :.taskclock 39,500 ± 1,942 22,500 ± >> 3,858 ms 0,57x (p = 0,000*) >> * = significant >> >> >> Comparing to a baseline without those recent improvements the overhead was >> almost the double: >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2050,000 ± 0,000 31,000 ± >> 2,673ms/op 1,61x (p = 0,000*) >> :.cycles 187047932,000 ± 3330400,381 102205427,650 ± >> 2192784,853 cycles 0,55x (p = 0,000*) >> :.instructions 408219060,350 ± 4031173,140 219415574,800 ± >> 376992,067 instructions 0,54x (p = 0,000*) >> :.taskclock 53,500 ± 4,249 22,500 ± >> 3,858 ms 0,42x (p = 0,000*) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove InternalError in favor of CCE LGTM, the CCE is the equivalent of should-not-reach-here. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2015014213
Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]
On Mon, 22 Apr 2024 14:11:41 GMT, Claes Redestad wrote: >> This switch expression in `Locale::createLocale` is causing a somewhat large >> startup regression on my local system. Desugaring to if statements seem like >> the right thing to do while we investigate ways to further reduce overheads >> in `SwitchBootstraps`. >> >> These numbers are against a baseline which include #18865 and #18845, which >> already improved the situation: >> >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2040,500 ± 1,942 31,000 ± >> 2,673ms/op 1,31x (p = 0,000*) >> :.cycles 143254849,000 ± 3398321,355 102205427,650 ± >> 2192784,853 cycles 0,71x (p = 0,000*) >> :.instructions 307138448,850 ± 2095834,550 219415574,800 ± >> 376992,067 instructions 0,71x (p = 0,000*) >> :.taskclock 39,500 ± 1,942 22,500 ± >> 3,858 ms 0,57x (p = 0,000*) >> * = significant >> >> >> Comparing to a baseline without those recent improvements the overhead was >> almost the double: >> >> Name Cnt Base Error Test >> Error Unit Change >> Perfstartup-Noop 2050,000 ± 0,000 31,000 ± >> 2,673ms/op 1,61x (p = 0,000*) >> :.cycles 187047932,000 ± 3330400,381 102205427,650 ± >> 2192784,853 cycles 0,55x (p = 0,000*) >> :.instructions 408219060,350 ± 4031173,140 219415574,800 ± >> 376992,067 instructions 0,54x (p = 0,000*) >> :.taskclock 53,500 ± 4,249 22,500 ± >> 3,858 ms 0,42x (p = 0,000*) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove InternalError in favor of CCE Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2014893801
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v16]
> Re-write the IndexOf code without the use of the pcmpestri instruction, only > using AVX2 instructions. This change accelerates String.IndexOf on average > 1.3x for AVX2. The benchmark numbers: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 48 commits: - Merge branch 'openjdk:master' into indexof - Remove infinite loop (used for debugging) - Merge branch 'openjdk:master' into indexof - Cleaned up, ready for review - Pre-cleanup code - Add JMH. Add 16-byte compares to arrays_equals - Better method for mask creation - Merge branch 'openjdk:master' into indexof - Most cleanup done. - Remove header dependency - ... and 38 more: https://git.openjdk.org/jdk/compare/3e65d90b...8e0ce70a - Changes: https://git.openjdk.org/jdk/pull/16753/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16753=15 Stats: 4903 lines in 19 files changed: 4549 ins; 241 del; 113 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]
> This switch expression in `Locale::createLocale` is causing a somewhat large > startup regression on my local system. Desugaring to if statements seem like > the right thing to do while we investigate ways to further reduce overheads > in `SwitchBootstraps`. > > These numbers are against a baseline which include #18865 and #18845, which > already improved the situation: > > > Name Cnt Base Error Test Error > Unit Change > Perfstartup-Noop 2040,500 ± 1,942 31,000 ± 2,673 >ms/op 1,31x (p = 0,000*) > :.cycles 143254849,000 ± 3398321,355 102205427,650 ± 2192784,853 > cycles 0,71x (p = 0,000*) > :.instructions 307138448,850 ± 2095834,550 219415574,800 ± 376992,067 > instructions 0,71x (p = 0,000*) > :.taskclock 39,500 ± 1,942 22,500 ± 3,858 > ms 0,57x (p = 0,000*) > * = significant > > > Comparing to a baseline without those recent improvements the overhead was > almost the double: > > Name Cnt Base Error Test Error > Unit Change > Perfstartup-Noop 2050,000 ± 0,000 31,000 ± 2,673 >ms/op 1,61x (p = 0,000*) > :.cycles 187047932,000 ± 3330400,381 102205427,650 ± 2192784,853 > cycles 0,55x (p = 0,000*) > :.instructions 408219060,350 ± 4031173,140 219415574,800 ± 376992,067 > instructions 0,54x (p = 0,000*) > :.taskclock 53,500 ± 4,249 22,500 ± 3,858 > ms 0,42x (p = 0,000*) > * = significant Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove InternalError in favor of CCE - Changes: - all: https://git.openjdk.org/jdk/pull/18882/files - new: https://git.openjdk.org/jdk/pull/18882/files/c4f5ee7c..0afa47ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18882=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18882=00-01 Stats: 6 lines in 1 file changed: 2 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18882.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18882/head:pull/18882 PR: https://git.openjdk.org/jdk/pull/18882
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container
On Fri, 19 Apr 2024 05:18:51 GMT, Laurence Cable wrote: > I think (I am agreeing with you Severin) that the goal of the heuristic is to > inform the JVM (and any associated serviceability tools) that the JVM is in a > resource constrained/managed execution context... "resource constrained" (my patch) vs. "managed" (this patch) is the difference of the two patches being discussed. Anyway in this patch one could unify naming across variables/parameters, the same value is called `_is_ro`, `is_read_only`, `ro_opt`, `read_only`, `ro`. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2069537759
Re: RFR: 8330802: Desugar switch in Locale::createLocale
On Mon, 22 Apr 2024 13:38:58 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/util/Locale.java line 1003:
>>
>>> 1001: return new Locale(lk.base, lk.exts);
>>> 1002: } else {
>>> 1003: throw new InternalError("should not happen");
>>
>> The default branch was required in the switch. Can we simply drop this
>> branch and have a ClassCastException to LocalKey instead?
>
> You mean like so:
>
>
> private static Locale createLocale(Object key) {
> if (key instanceof BaseLocale base) {
> return new Locale(base, null);
> }
> LocaleKey lk = (LocaleKey)key;
> return new Locale(lk.base, lk.exts);
> }
> ```
> ?
>
> I think this should be alright. The type of the "impossibly" thrown
> exception would differ.
Yes, just like how jli handles the Class/MethodType union type arguments.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18882#discussion_r1574791014
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v7]
On Mon, 22 Apr 2024 08:46:53 GMT, Per Minborg wrote:
>> While `SymbolLookup` correctly uses an `Optional` return to denote whether a
>> symbol has been found by the lookup or not (which enables composition of
>> symbol lookups), many clients end up just calling `Optional::get`, or
>> `Optional::orElseThrow()` on the result.
>>
>> This PR proposes to add a convenience method `SymbolLookup::findOrThrow`
>> that will do a lookup and, if no symbol can be found, throws an
>> `IllegalArgumentException` with a relevant exception message.
>
> Per Minborg 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 12 additional
> commits since the last revision:
>
> - Simplify tests
> - Add a test for null arg
> - Add a test for findOrThrow()
> - Merge branch 'master' into symbol-lookup-findorthrow
> - Change exception type
> - Update src/java.base/share/classes/java/lang/foreign/package-info.java
>
>Co-authored-by: Jorn Vernee
> - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java
>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java
>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Fix typo
> - Update after PR comments
> - ... and 2 more: https://git.openjdk.org/jdk/compare/9a68d47d...0e06e0d6
test/jdk/java/foreign/loaderLookup/TestSymbolLookupFindOrThrow.java line 41:
> 39:
> 40: static {
> 41: System.loadLibrary("Foo");
Where is this lib defined?
test/jdk/java/foreign/loaderLookup/TestSymbolLookupFindOrThrow.java line 58:
> 56: @Test
> 57: void findOrThrowNullArg() {
> 58: assertThrows(NullPointerException.class, () ->
I believe this should already be checked by the TestNulls test - which is a
test that automatically checks that _all_ API methods handle nulls accordingly.
Please check that, and maybe remove this?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1574788219
PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1574786946
Re: RFR: 8330802: Desugar switch in Locale::createLocale
On Mon, 22 Apr 2024 13:05:47 GMT, Chen Liang wrote:
>> This switch expression in `Locale::createLocale` is causing a somewhat large
>> startup regression on my local system. Desugaring to if statements seem like
>> the right thing to do while we investigate ways to further reduce overheads
>> in `SwitchBootstraps`.
>>
>> These numbers are against a baseline which include #18865 and #18845, which
>> already improved the situation:
>>
>>
>> Name Cnt Base Error Test
>> Error Unit Change
>> Perfstartup-Noop 2040,500 ± 1,942 31,000 ±
>> 2,673ms/op 1,31x (p = 0,000*)
>> :.cycles 143254849,000 ± 3398321,355 102205427,650 ±
>> 2192784,853 cycles 0,71x (p = 0,000*)
>> :.instructions 307138448,850 ± 2095834,550 219415574,800 ±
>> 376992,067 instructions 0,71x (p = 0,000*)
>> :.taskclock 39,500 ± 1,942 22,500 ±
>> 3,858 ms 0,57x (p = 0,000*)
>> * = significant
>>
>>
>> Comparing to a baseline without those recent improvements the overhead was
>> almost the double:
>>
>> Name Cnt Base Error Test
>> Error Unit Change
>> Perfstartup-Noop 2050,000 ± 0,000 31,000 ±
>> 2,673ms/op 1,61x (p = 0,000*)
>> :.cycles 187047932,000 ± 3330400,381 102205427,650 ±
>> 2192784,853 cycles 0,55x (p = 0,000*)
>> :.instructions 408219060,350 ± 4031173,140 219415574,800 ±
>> 376992,067 instructions 0,54x (p = 0,000*)
>> :.taskclock 53,500 ± 4,249 22,500 ±
>> 3,858 ms 0,42x (p = 0,000*)
>> * = significant
>
> src/java.base/share/classes/java/util/Locale.java line 1003:
>
>> 1001: return new Locale(lk.base, lk.exts);
>> 1002: } else {
>> 1003: throw new InternalError("should not happen");
>
> The default branch was required in the switch. Can we simply drop this branch
> and have a ClassCastException to LocalKey instead?
You mean like so:
private static Locale createLocale(Object key) {
if (key instanceof BaseLocale base) {
return new Locale(base, null);
}
LocaleKey lk = (LocaleKey)key;
return new Locale(lk.base, lk.exts);
}
```
?
I think this should be alright. The type of the "impossibly" thrown exception
would differ.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18882#discussion_r1574776021
Re: RFR: 8330802: Desugar switch in Locale::createLocale
On Mon, 22 Apr 2024 10:34:29 GMT, Claes Redestad wrote:
> This switch expression in `Locale::createLocale` is causing a somewhat large
> startup regression on my local system. Desugaring to if statements seem like
> the right thing to do while we investigate ways to further reduce overheads
> in `SwitchBootstraps`.
>
> These numbers are against a baseline which include #18865 and #18845, which
> already improved the situation:
>
>
> Name Cnt Base Error Test Error
> Unit Change
> Perfstartup-Noop 2040,500 ± 1,942 31,000 ± 2,673
>ms/op 1,31x (p = 0,000*)
> :.cycles 143254849,000 ± 3398321,355 102205427,650 ± 2192784,853
> cycles 0,71x (p = 0,000*)
> :.instructions 307138448,850 ± 2095834,550 219415574,800 ± 376992,067
> instructions 0,71x (p = 0,000*)
> :.taskclock 39,500 ± 1,942 22,500 ± 3,858
> ms 0,57x (p = 0,000*)
> * = significant
>
>
> Comparing to a baseline without those recent improvements the overhead was
> almost the double:
>
> Name Cnt Base Error Test Error
> Unit Change
> Perfstartup-Noop 2050,000 ± 0,000 31,000 ± 2,673
>ms/op 1,61x (p = 0,000*)
> :.cycles 187047932,000 ± 3330400,381 102205427,650 ± 2192784,853
> cycles 0,55x (p = 0,000*)
> :.instructions 408219060,350 ± 4031173,140 219415574,800 ± 376992,067
> instructions 0,54x (p = 0,000*)
> :.taskclock 53,500 ± 4,249 22,500 ± 3,858
> ms 0,42x (p = 0,000*)
> * = significant
src/java.base/share/classes/java/util/Locale.java line 1003:
> 1001: return new Locale(lk.base, lk.exts);
> 1002: } else {
> 1003: throw new InternalError("should not happen");
The default branch was required in the switch. Can we simply drop this branch
and have a ClassCastException to LocalKey instead?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18882#discussion_r1574727393
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
> This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy (as can > happen if debugging code is added to ForkJoinPool) and preemption when > compensating (as can happen when substituting a heap buffer with a direct > buffer in some I/O operations). This part is a pre-requisite to the changes > to better support object monitor there are more places where preemption is > possible and this quickly leads to unbalanced begin/end. > > The changes have been baking in loom repo for some time. Alan Bateman 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 five additional commits since the last revision: - Merge - Sync up from loom repo, update copyright headers - Merge - Merge - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/18598/files - new: https://git.openjdk.org/jdk/pull/18598/files/a21a8d6b..0d99fe0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18598=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18598=00-01 Stats: 35845 lines in 976 files changed: 19000 ins; 10527 del; 6318 mod Patch: https://git.openjdk.org/jdk/pull/18598.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18598/head:pull/18598 PR: https://git.openjdk.org/jdk/pull/18598
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy (as can > happen if debugging code is added to ForkJoinPool) and preemption when > compensating (as can happen when substituting a heap buffer with a direct > buffer in some I/O operations). This part is a pre-requisite to the changes > to better support object monitor there are more places where preemption is > possible and this quickly leads to unbalanced begin/end. > > The changes have been baking in loom repo for some time. The changes are updated to pin when attempting to increase parallelism. That removes the possibility of preemption in begingBlocking for now so hopefully this is easier to understand. Also removed the transferTo overloads for now, that would make it easier too but we may want to come back to them in the future. - PR Comment: https://git.openjdk.org/jdk/pull/18598#issuecomment-2069298348
Re: RFR: 8330802: Desugar switch in Locale::createLocale
On Mon, 22 Apr 2024 10:34:29 GMT, Claes Redestad wrote: > This switch expression in `Locale::createLocale` is causing a somewhat large > startup regression on my local system. Desugaring to if statements seem like > the right thing to do while we investigate ways to further reduce overheads > in `SwitchBootstraps`. > > These numbers are against a baseline which include #18865 and #18845, which > already improved the situation: > > > Name Cnt Base Error Test Error > Unit Change > Perfstartup-Noop 2040,500 ± 1,942 31,000 ± 2,673 >ms/op 1,31x (p = 0,000*) > :.cycles 143254849,000 ± 3398321,355 102205427,650 ± 2192784,853 > cycles 0,71x (p = 0,000*) > :.instructions 307138448,850 ± 2095834,550 219415574,800 ± 376992,067 > instructions 0,71x (p = 0,000*) > :.taskclock 39,500 ± 1,942 22,500 ± 3,858 > ms 0,57x (p = 0,000*) > * = significant > > > Comparing to a baseline without those recent improvements the overhead was > almost the double: > > Name Cnt Base Error Test Error > Unit Change > Perfstartup-Noop 2050,000 ± 0,000 31,000 ± 2,673 >ms/op 1,61x (p = 0,000*) > :.cycles 187047932,000 ± 3330400,381 102205427,650 ± 2192784,853 > cycles 0,55x (p = 0,000*) > :.instructions 408219060,350 ± 4031173,140 219415574,800 ± 376992,067 > instructions 0,54x (p = 0,000*) > :.taskclock 53,500 ± 4,249 22,500 ± 3,858 > ms 0,42x (p = 0,000*) > * = significant Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2014569606
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Mon, 22 Apr 2024 11:30:41 GMT, Matthias Baesken wrote: > Hi, any additional comments / reviews ? Thanks Matthias It still looks like left over trace messages from a debugging session, need to think about about what tracing make sense here. - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2069209127
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Fri, 19 Apr 2024 10:07:22 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove launcher executable path trace output Hi, any additional comments / reviews ? Thanks Matthias - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2069158463
RFR: 8330802: Desugar switch in Locale::createLocale
This switch expression in `Locale::createLocale` is causing a somewhat large startup regression on my local system. Desugaring to if statements seem like the right thing to do while we investigate ways to further reduce overheads in `SwitchBootstraps`. These numbers are against a baseline which include #18865 and #18845, which already improved the situation: Name Cnt Base Error Test Error Unit Change Perfstartup-Noop 2040,500 ± 1,942 31,000 ± 2,673 ms/op 1,31x (p = 0,000*) :.cycles 143254849,000 ± 3398321,355 102205427,650 ± 2192784,853 cycles 0,71x (p = 0,000*) :.instructions 307138448,850 ± 2095834,550 219415574,800 ± 376992,067 instructions 0,71x (p = 0,000*) :.taskclock 39,500 ± 1,942 22,500 ± 3,858 ms 0,57x (p = 0,000*) * = significant Comparing to a baseline without those recent improvements the overhead was almost the double: Name Cnt Base Error Test Error Unit Change Perfstartup-Noop 2050,000 ± 0,000 31,000 ± 2,673 ms/op 1,61x (p = 0,000*) :.cycles 187047932,000 ± 3330400,381 102205427,650 ± 2192784,853 cycles 0,55x (p = 0,000*) :.instructions 408219060,350 ± 4031173,140 219415574,800 ± 376992,067 instructions 0,54x (p = 0,000*) :.taskclock 53,500 ± 4,249 22,500 ± 3,858 ms 0,42x (p = 0,000*) * = significant - Commit messages: - Merge branch 'master' into deswitch - Desugar switch in Locale::createLocale Changes: https://git.openjdk.org/jdk/pull/18882/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18882=00 Issue: https://bugs.openjdk.org/browse/JDK-8330802 Stats: 7 lines in 1 file changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18882.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18882/head:pull/18882 PR: https://git.openjdk.org/jdk/pull/18882
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v6]
On Thu, 18 Apr 2024 14:50:30 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Minor SimpleStringBuilderStrategy:: overhead reduction This one is still waiting for a review. @mlchung @asotona: Alan asked me to use the ClassFile API here. As it's only used in what's effectively a slow path it shouldn't be blocked by the startup investigations we're doing there, right? - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2068862978
Re: RFR: 8330595: Invoke ObjectMethods::bootstrap method exactly [v3]
On Fri, 19 Apr 2024 07:42:19 GMT, Claes Redestad wrote: >> Investigating a recent regression in mainline I realized we have an >> opportunity to improve the bootstrap overheads of ObjectMethods::bootstrap >> by using `invokeExact` in a way similar to what we already do for LMF and >> SCF BSMs. This avoids generating type checking adapters and equates to a >> one-off startup win of around 5ms in any app that ever has the need to spin >> up a toString, equals or hashCode method on a record. > > Claes Redestad has updated the pull request incrementally with two additional > commits since the last revision: > > - Copyright > - Formatting Thanks for the reviews, Mandy! - PR Comment: https://git.openjdk.org/jdk/pull/18845#issuecomment-2068853277
Integrated: 8330595: Invoke ObjectMethods::bootstrap method exactly
On Thu, 18 Apr 2024 20:11:15 GMT, Claes Redestad wrote: > Investigating a recent regression in mainline I realized we have an > opportunity to improve the bootstrap overheads of ObjectMethods::bootstrap by > using `invokeExact` in a way similar to what we already do for LMF and SCF > BSMs. This avoids generating type checking adapters and equates to a one-off > startup win of around 5ms in any app that ever has the need to spin up a > toString, equals or hashCode method on a record. This pull request has now been integrated. Changeset: 35b30c81 Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/35b30c81e0153a12881e622b861ee38c8166ef72 Stats: 17 lines in 1 file changed: 15 ins; 1 del; 1 mod 8330595: Invoke ObjectMethods::bootstrap method exactly Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/18845
Integrated: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs
On Fri, 19 Apr 2024 13:23:53 GMT, Claes Redestad wrote: > We can reduce overhead of first use of a switch bootstrap by moving > `typePairToName` into `TypePairs` and by explicitly overriding `hashCode` and > `equals`. The first change avoids loading and initializing the `TypePairs` > class in actual cases, the second remove some excess code generation from > happening on first use. This pull request has now been integrated. Changeset: 3d62bbf4 Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/3d62bbf4f2ea1b37d59c8307225239a88d9e66c0 Stats: 18 lines in 1 file changed: 14 ins; 3 del; 1 mod 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs Reviewed-by: jlahoda, mchung - PR: https://git.openjdk.org/jdk/pull/18865
Re: RFR: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs [v3]
On Sat, 20 Apr 2024 07:39:53 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 685:
>>
>>> 683: record TypePairs(Class from, Class to) {
>>> 684:
>>> 685: private static final Map typePairToName =
>>> initialize();
>>
>> If `TypePairs.typePairToName` is never modified after initialisation, then
>> it should probably be made immutable:
>> Suggestion:
>>
>> private static final Map typePairToName =
>> Map.copyOf(initialize());
>
> If you really think about it, the `initialize` method itself is somewhat
> problematic, as it's initializing with byte/short/char on the left, all of
> which are already converted to int in the of() factory. This should be done
> in a separate issue.
Yes, the "redirected" mappings can simply be removed in the current
implementation.
Using `Map.copyOf` should be ok to nail down the read-only intent.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18865#discussion_r1574385797
Re: RFR: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs [v3]
> We can reduce overhead of first use of a switch bootstrap by moving > `typePairToName` into `TypePairs` and by explicitly overriding `hashCode` and > `equals`. The first change avoids loading and initializing the `TypePairs` > class in actual cases, the second remove some excess code generation from > happening on first use. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Revert changes, splitting these out to a separate PR - Changes: - all: https://git.openjdk.org/jdk/pull/18865/files - new: https://git.openjdk.org/jdk/pull/18865/files/b6d29f2a..6226f1b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18865=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18865=01-02 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18865.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18865/head:pull/18865 PR: https://git.openjdk.org/jdk/pull/18865
Re: RFR: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs [v2]
> We can reduce overhead of first use of a switch bootstrap by moving > `typePairToName` into `TypePairs` and by explicitly overriding `hashCode` and > `equals`. The first change avoids loading and initializing the `TypePairs` > class in actual cases, the second remove some excess code generation from > happening on first use. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove redundant mappings; store an immutable copy - Changes: - all: https://git.openjdk.org/jdk/pull/18865/files - new: https://git.openjdk.org/jdk/pull/18865/files/57d50841..b6d29f2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18865=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18865=00-01 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18865.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18865/head:pull/18865 PR: https://git.openjdk.org/jdk/pull/18865
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v7]
> While `SymbolLookup` correctly uses an `Optional` return to denote whether a > symbol has been found by the lookup or not (which enables composition of > symbol lookups), many clients end up just calling `Optional::get`, or > `Optional::orElseThrow()` on the result. > > This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that > will do a lookup and, if no symbol can be found, throws an > `IllegalArgumentException` with a relevant exception message. Per Minborg 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 12 additional commits since the last revision: - Simplify tests - Add a test for null arg - Add a test for findOrThrow() - Merge branch 'master' into symbol-lookup-findorthrow - Change exception type - Update src/java.base/share/classes/java/lang/foreign/package-info.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Fix typo - Update after PR comments - ... and 2 more: https://git.openjdk.org/jdk/compare/76cd531f...0e06e0d6 - Changes: - all: https://git.openjdk.org/jdk/pull/18474/files - new: https://git.openjdk.org/jdk/pull/18474/files/e2f6c4c9..0e06e0d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18474=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18474=05-06 Stats: 91042 lines in 1455 files changed: 42444 ins; 38886 del; 9712 mod Patch: https://git.openjdk.org/jdk/pull/18474.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18474/head:pull/18474 PR: https://git.openjdk.org/jdk/pull/18474
