Re: RFR: 8304914: Use OperatingSystem, Architecture, and Version in jpackage [v2]
On Fri, 5 May 2023 14:12:21 GMT, Roger Riggs wrote:
>> Refactor the Platform class in jdk.jpackage to use the internal
>> OperatingSystem, Architecture, and Version classes.
>> The OperatingSystem.isXXX() and Architecture.isYYY() methods replace
>> comparisons in the Platform class.
>> The checks of the os.version are replaced but may not be needed if OpenJDK
>> no longer supports them.
>>
>> It is recommended to remove os version checks that apply only to Mac
>> versions before 10.15.
>> Mac OS X 10.15 is the oldest version supported.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Minor source code style cleanup
src/jdk.jpackage/share/classes/jdk/jpackage/internal/CLIHelp.java line 51:
> 49: Log.info(I18N.getString("MSG_Help_no_args"));
> 50: } else {
> 51: OperatingSystem platform = OperatingSystem.current();
Should we add if (Log.isVerbose) back? Otherwise default case in switch
statement never executed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1192826773
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
On Fri, 12 May 2023 15:24:19 GMT, Volker Simonis wrote:
> Since JDK13, executing commands in a sub-process defaults to the so called
> `POSIX_SPAWN` launching mechanism (i.e.
> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
> using `posix_spawn(3)` to firstly start a tiny helper program called
> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
> command data from the parent Java process over a Unix pipe and finally
> executes (i.e. `execvp(3)`) the requested command.
>
> In cases where the parent process terminates abnormally before `jspawnhelper`
> has read all the expected data from the pipe, `jspawnhelper` will block
> indefinitely on the reading end of the pipe. This is especially harmful if
> the parent process had open sockets, because in that case, the forked
> `jspawnhelper` process will inherit them and keep all the corresponding ports
> open effectively preventing other processes to bind to them. Notice that this
> is not an abstract scenario. We've observed this regularly in production with
> services which couldn't be restarted after a crash after migrating to JDK 17.
>
> The fix of the issue is rather trivial. `jspawnhelper` has to close its
> writing end of the pipe which connects it with the parent Java process
> *before* starting to read from that pipe such that reads from the pipe can
> immediately return with EOF if the parent process terminates abnormally.
>
> Also did some cleanup:
> - in `jspawnhelper.c` correctly chek the result of `readFully()`
> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
> write the command data from the parent process to `jspawnhelper`
> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
> that's long gone.
Looks ok.
Is it practical to write a test for this situation?
Can I assume you've validated the improvement as a remedy for the observed
hangs?
-
PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1546332294
Re: RFR: 8284493: Improve computeNextExponential tail performance and accuracy [v20]
On Fri, 12 May 2023 20:41:32 GMT, Chris Hennick wrote: >> This PR improves both the worst-case performance of `nextExponential` and >> `nextGaussian` and the distribution of output at the tails. It fixes the >> following imperfections: >> >> * Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a >> rounding error to accumulate at the tail of the distribution (probably >> starting around `2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1`); this PR >> fixes that by tracking the multiple of exponentialX0 as a long. (This >> distortion is worst when `x > 0x1.0p56` since in that case, a rounding error >> means `extra + x == extra`. >> * Reduces several equations using `Math.fma`. (This will almost certainly >> improve performance, and may or may not improve output distribution.) >> * Uses the newly-extracted `computeWinsorizedNextExponential` function to >> prevent `nextGaussian` from going into the `nextExponential` tail twice. > > Chris Hennick has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/patch-1' into patch-1 > - Optimize: move some code out of the fast path Latest change looks good. I noted the improvement to the comment beginning "We didn't use the upper part of U1 after all". - PR Comment: https://git.openjdk.org/jdk/pull/8131#issuecomment-1546292204
Re: RFR: 8299340: CreateProcessW lpCommandLine must be mutable [v2]
On Fri, 12 May 2023 19:38:53 GMT, Roger Riggs wrote: >> Launching of processes on Windows using `ProcessCreateW` with a Unicode >> character set requires the buffer to be writable. An access violation might >> occur if `ProcessCreateW` writes to the command line string. The current >> implementation fetches the command line string using JNI GetStringChars >> returning a buffer that should not be modified. The code is unchanged since >> 2015. There have not been any reported faults in that time. >> >> This change copies the command line to a separately allocation mutable >> buffer to satisfy the Windows requirement. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Add unicode null to native command line copy LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13894#pullrequestreview-1425138318
Re: RFR: 8284493: Improve computeNextExponential tail performance and accuracy [v19]
On Thu, 6 Apr 2023 18:07:29 GMT, Chris Hennick wrote: >> This PR improves both the worst-case performance of `nextExponential` and >> `nextGaussian` and the distribution of output at the tails. It fixes the >> following imperfections: >> >> * Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a >> rounding error to accumulate at the tail of the distribution (probably >> starting around `2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1`); this PR >> fixes that by tracking the multiple of exponentialX0 as a long. (This >> distortion is worst when `x > 0x1.0p56` since in that case, a rounding error >> means `extra + x == extra`. >> * Reduces several equations using `Math.fma`. (This will almost certainly >> improve performance, and may or may not improve output distribution.) >> * Uses the newly-extracted `computeWinsorizedNextExponential` function to >> prevent `nextGaussian` from going into the `nextExponential` tail twice. > > Chris Hennick has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' of https://git.openjdk.org/jdk into patch-1 > - Merge branch 'master' of https://git.openjdk.org/jdk into patch-1 > - Merge branch 'master' into patch-1 > - Update copyright date in RandomNext.java > - Update copyright date in RandomGeneratorNext.java > - Update copyright date in RandomGeneratorExponentialGaussian.java > - Update copyright date in RandomSupport.java > - Add parameter to enable/disable fixed PRNG seed > - Rewrite Javadoc > - Simplify Javadoc description > - ... and 7 more: https://git.openjdk.org/jdk/compare/08fbb7bb...2470c00a Moved the code after the fast-path return as requested. - PR Comment: https://git.openjdk.org/jdk/pull/8131#issuecomment-1546259257
Re: RFR: 8284493: Improve computeNextExponential tail performance and accuracy [v20]
> This PR improves both the worst-case performance of `nextExponential` and > `nextGaussian` and the distribution of output at the tails. It fixes the > following imperfections: > > * Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a > rounding error to accumulate at the tail of the distribution (probably > starting around `2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1`); this PR > fixes that by tracking the multiple of exponentialX0 as a long. (This > distortion is worst when `x > 0x1.0p56` since in that case, a rounding error > means `extra + x == extra`. > * Reduces several equations using `Math.fma`. (This will almost certainly > improve performance, and may or may not improve output distribution.) > * Uses the newly-extracted `computeWinsorizedNextExponential` function to > prevent `nextGaussian` from going into the `nextExponential` tail twice. Chris Hennick has updated the pull request incrementally with two additional commits since the last revision: - Merge remote-tracking branch 'origin/patch-1' into patch-1 - Optimize: move some code out of the fast path - Changes: - all: https://git.openjdk.org/jdk/pull/8131/files - new: https://git.openjdk.org/jdk/pull/8131/files/2470c00a..e4714ec9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=8131=19 - incr: https://webrevs.openjdk.org/?repo=jdk=8131=18-19 Stats: 20 lines in 1 file changed: 10 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/8131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/8131/head:pull/8131 PR: https://git.openjdk.org/jdk/pull/8131
Integrated: 6714245: [Col] Collator - Faster Comparison for identical strings.
On Thu, 11 May 2023 18:27:28 GMT, Justin Lu wrote: > Please review this PR which adds an initial equality check to > `RuleBasedCollator.compare(String source, String target)`. > > This speeds up the operation for equal input Strings, as it bypasses Collator > rule comparisons of each element for both of the input Strings. This pull request has now been integrated. Changeset: 4441a230 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/4441a2306fb12f60ac879f7fda6c7446ac130dcb Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 6714245: [Col] Collator - Faster Comparison for identical strings. Reviewed-by: rriggs, naoto - PR: https://git.openjdk.org/jdk/pull/13938
Re: RFR: 8304914: Use OperatingSystem, Architecture, and Version in jpackage [v2]
On Fri, 5 May 2023 14:12:21 GMT, Roger Riggs wrote: >> Refactor the Platform class in jdk.jpackage to use the internal >> OperatingSystem, Architecture, and Version classes. >> The OperatingSystem.isXXX() and Architecture.isYYY() methods replace >> comparisons in the Platform class. >> The checks of the os.version are replaced but may not be needed if OpenJDK >> no longer supports them. >> >> It is recommended to remove os version checks that apply only to Mac >> versions before 10.15. >> Mac OS X 10.15 is the oldest version supported. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Minor source code style cleanup Marked as reviewed by asemenyuk (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13586#pullrequestreview-1425079644
Re: RFR: 8284493: Improve computeNextExponential tail performance and accuracy [v19]
On Thu, 6 Apr 2023 18:07:29 GMT, Chris Hennick wrote: >> This PR improves both the worst-case performance of `nextExponential` and >> `nextGaussian` and the distribution of output at the tails. It fixes the >> following imperfections: >> >> * Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a >> rounding error to accumulate at the tail of the distribution (probably >> starting around `2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1`); this PR >> fixes that by tracking the multiple of exponentialX0 as a long. (This >> distortion is worst when `x > 0x1.0p56` since in that case, a rounding error >> means `extra + x == extra`. >> * Reduces several equations using `Math.fma`. (This will almost certainly >> improve performance, and may or may not improve output distribution.) >> * Uses the newly-extracted `computeWinsorizedNextExponential` function to >> prevent `nextGaussian` from going into the `nextExponential` tail twice. > > Chris Hennick has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' of https://git.openjdk.org/jdk into patch-1 > - Merge branch 'master' of https://git.openjdk.org/jdk into patch-1 > - Merge branch 'master' into patch-1 > - Update copyright date in RandomNext.java > - Update copyright date in RandomGeneratorNext.java > - Update copyright date in RandomGeneratorExponentialGaussian.java > - Update copyright date in RandomSupport.java > - Add parameter to enable/disable fixed PRNG seed > - Rewrite Javadoc > - Simplify Javadoc description > - ... and 7 more: https://git.openjdk.org/jdk/compare/08fbb7bb...2470c00a I have now reviewed all the changes in file .../jdk/internal/util/random/RandomSupport.java . It appears that the method formerly called computeWinsorizedNextExponential has been renamed to computeNextExponentialSoftCapped, and that is a good change. All the changes to the executable code appear to be sound (and, yes, I believe I still remember how all this code is supposed to work—even while admitting that I wrote it some years ago and that a couple of embarrassing mistakes were later discovered and repaired, thank goodness). I looked especially carefully at the change of representation for the variable "extra" from double to long and the new uses of fma. I went back and looked at email exchanges I had with Joe Darcy on May 10, 2022, and I see that many of the concerns I expressed at that time have been addressed. I do suggest that for extra clarity, the declaration and computation of maxExtraMinus1 at lines 1206--1212 be moved down below to after line 1222, just below the comment that begins "We didn't use the upper part of U1 after all". It may be that good optimizing Java compilers perform this code motion anyway (the code in lines 1213–1221 does not refer to maxExtraMinus1), but it would help a human reader to understand that this code is not part of and does not need to be part of the fast path through the code. Moreover, though a compiler cannot tell that it's okay to move the comparison of maxValue to 0.0 (and the possible forced return of 0.0) off the fast path, I argue that it is indeed okay to do so, because it is always permitted to return a value larger than maxValue, and the fast path does always return a nonnegative value. So I actually argue to move three more lines (in all, lines 1203–1212) to after line 1222. In all other respects, I recommend that this set of changes be adopted exactly as is. If making the one change I suggested above might cause adoption to slip from JDK21 to JDK22 (perhaps because of a need for retesting), then I would suggest adopting the code exactly as is and then scheduling the suggested change for JDK22, because the suggested change improves clarity and code speed but should not change the advertised functionality at all. - PR Comment: https://git.openjdk.org/jdk/pull/8131#issuecomment-1546202844
RFR: 8308022: update for deprecated sprintf for java.base
Hi, May I have this update reviewed? The sprintf is deprecated in Xcode 14, and Microsoft Virtual Studio, because of security concerns. The issue was addressed in [JDK-8296812](https://bugs.openjdk.org/browse/JDK-8296812) for building failure, and [JDK-8299378](https://bugs.openjdk.org/browse/JDK-8299378)/[JDK-8299635](https://bugs.openjdk.org/browse/JDK-8299635)/[JDK-8301132](https://bugs.openjdk.org/browse/JDK-8301132) for testing issues . This is a break-down update for sprintf uses in the java.base module. Thanks, Xuelei - Commit messages: - update copyright year - 8308022: update for deprecated sprintf for java.base Changes: https://git.openjdk.org/jdk/pull/13963/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13963=00 Issue: https://bugs.openjdk.org/browse/JDK-8308022 Stats: 44 lines in 9 files changed: 5 ins; 0 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/13963.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13963/head:pull/13963 PR: https://git.openjdk.org/jdk/pull/13963
Re: RFR: 8299340: CreateProcessW lpCommandLine must be mutable [v2]
> Launching of processes on Windows using `ProcessCreateW` with a Unicode > character set requires the buffer to be writable. An access violation might > occur if `ProcessCreateW` writes to the command line string. The current > implementation fetches the command line string using JNI GetStringChars > returning a buffer that should not be modified. The code is unchanged since > 2015. There have not been any reported faults in that time. > > This change copies the command line to a separately allocation mutable buffer > to satisfy the Windows requirement. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Add unicode null to native command line copy - Changes: - all: https://git.openjdk.org/jdk/pull/13894/files - new: https://git.openjdk.org/jdk/pull/13894/files/b8e49a65..41b7f7d1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13894=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13894=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13894.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13894/head:pull/13894 PR: https://git.openjdk.org/jdk/pull/13894
Re: RFR: 8305895: Implementation: JEP 450: Compact Object Headers (Experimental)
On Fri, 12 May 2023 18:03:13 GMT, Coleen Phillimore wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are building on #10907, #13582 and #13779 to >> protect the relevant (upper 32) bits of the mark-word. Significant parts of >> this PR deal with loading the compressed Klass* from the mark-word, and >> dealing with (monitor-)locked objects. When the object is monitor-locked, we >> load the displaced mark-word from the monitor, and load the compressed >> Klass* from there. This PR also changes some code paths (mostly in GCs) to >> be more careful when accessing Klass* (or mark-word or size) to be able to >> fetch it from the forwardee in case the object is forwarded, and/or reach >> through to the monitor when the object is locked by a monitor. >> - The identity hash-code is narrowed to 25 bits. >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will can now store their length at offset 8. Due to alignment >> restrictions, array elements will still start at offset 16. #11044 will >> resolve that restriction and allow array elements to start at offset 12 >> (except for long, double and uncompressed oops, which are still required to >> start at an element-aligned offset). >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. >> >> Testing: >> (+UseCompactObjectHeaders tests are run with the flag hard-patched into the >> build, to also catch @flagless tests, and to avoid mismatches with CDS - see >> above.) >> - [x] tier1 (x86_64) >> - [x] tier2 (x86_64) >> - [x] tier3 (x86_64) >> - [ ] tier4 (x86_64) >> - [x] tier1 (aarch64) >> - [x] tier2 (aarch64) >> - [x] tier3 (aarch64) >> - [ ] tier4 (aarch64) >> - [ ] tier1 (x86_64) +UseCompactObjectHeaders >> - [ ] tier2 (x86_64) +UseCompactObjectHeaders >> - [ ] tier3 (x86_64) +UseCompactObjectHeaders >> - [ ] tier4 (x86_64) +UseCompactObjectHeaders >> - [ ] tier1 (aarch64) +UseCompactObjectHeaders >> - [ ] tier2 (aarch64) +UseCompactObjectHeaders >> - [ ] tier3 (aarch64) +UseCompactObjectHeaders >> - [ ] tier4 (aarch64) +UseCompactObjectHeaders > > src/hotspot/share/cds/archiveBuilder.cpp line 726: > >> 724: >> k->set_prototype_header(markWord::prototype().set_narrow_klass(nk)); >> 725: } >> 726: #endif //_LP64 > > If CDS is turned off for UseCompactObjectHeaders, I don't understand this > change or the one to archiveHeapWriter. -Xshare:dump objects would be the > wrong size. If CDS is not supported, then there should be something in > arguments.cpp that gives an error for that. And write a test for that error > of mixing and matching. Yeah, we do have code in arguments.cpp that turns off CDS if the wrong setting is used (i.e. the opposite of the default setting). If you hard-code UseCompactObjectHeaders to be true, then the archives will be written in the compact layout, and can be read. That's what the changes in share/cds implement. (Note: I regularily hard-patch the UseCompactObjectHeaders flag to be true for testing, because that also catches all the @flagless tests, and I know that Daniel does that, too.) We disabled CDS jtreg tests when passing +UseCompactObjectHeaders via cmd line, because we would see a lot of test failures because of archive format mismatch. I am not sure if that's a good way to deal with that. - PR Review Comment: https://git.openjdk.org/jdk/pull/13961#discussion_r1192698676
Re: RFR: 8305895: Implementation: JEP 450: Compact Object Headers (Experimental)
On Fri, 12 May 2023 17:27:25 GMT, Roman Kennke wrote:
> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>
> Main changes:
> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
> changes in this PR are protected by this flag.
> - The compressed Klass* can now be stored in the mark-word of objects. In
> order to be able to do this, we are building on #10907, #13582 and #13779 to
> protect the relevant (upper 32) bits of the mark-word. Significant parts of
> this PR deal with loading the compressed Klass* from the mark-word, and
> dealing with (monitor-)locked objects. When the object is monitor-locked, we
> load the displaced mark-word from the monitor, and load the compressed Klass*
> from there. This PR also changes some code paths (mostly in GCs) to be more
> careful when accessing Klass* (or mark-word or size) to be able to fetch it
> from the forwardee in case the object is forwarded, and/or reach through to
> the monitor when the object is locked by a monitor.
> - The identity hash-code is narrowed to 25 bits.
> - Instances can now have their base-offset (the offset where the field
> layouter starts to place fields) at offset 8 (instead of 12 or 16).
> - Arrays will can now store their length at offset 8. Due to alignment
> restrictions, array elements will still start at offset 16. #11044 will
> resolve that restriction and allow array elements to start at offset 12
> (except for long, double and uncompressed oops, which are still required to
> start at an element-aligned offset).
> - CDS can now write and read archives with the compressed header. However,
> it is not possible to read an archive that has been written with an opposite
> setting of UseCompactObjectHeaders.
>
> Testing:
> (+UseCompactObjectHeaders tests are run with the flag hard-patched into the
> build, to also catch @flagless tests, and to avoid mismatches with CDS - see
> above.)
> - [x] tier1 (x86_64)
> - [x] tier2 (x86_64)
> - [x] tier3 (x86_64)
> - [ ] tier4 (x86_64)
> - [x] tier1 (aarch64)
> - [x] tier2 (aarch64)
> - [x] tier3 (aarch64)
> - [ ] tier4 (aarch64)
> - [ ] tier1 (x86_64) +UseCompactObjectHeaders
> - [ ] tier2 (x86_64) +UseCompactObjectHeaders
> - [ ] tier3 (x86_64) +UseCompactObjectHeaders
> - [ ] tier4 (x86_64) +UseCompactObjectHeaders
> - [ ] tier1 (aarch64) +UseCompactObjectHeaders
> - [ ] tier2 (aarch64) +UseCompactObjectHeaders
> - [ ] tier3 (aarch64) +UseCompactObjectHeaders
> - [ ] tier4 (aarch64) +UseCompactObjectHeaders
These changes are an improvement.
src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp line 193:
> 191: movl(Address(obj, arrayOopDesc::length_offset_in_bytes() +
> sizeof(jint)), t1);
> 192: }
> 193: #endif
This endif should go after UseCompressedClassPointers conditional, and
consolidate to one set of #ifdef _LP64.
src/hotspot/cpu/x86/macroAssembler_x86.cpp line 5126:
> 5124: assert(UseCompactObjectHeaders, "expect compact object headers");
> 5125:
> 5126: if (!UseCompactObjectHeaders) {
Now this isn't needed, right?
src/hotspot/share/cds/archiveBuilder.cpp line 726:
> 724:
> k->set_prototype_header(markWord::prototype().set_narrow_klass(nk));
> 725: }
> 726: #endif //_LP64
If CDS is turned off for UseCompactObjectHeaders, I don't understand this
change or the one to archiveHeapWriter. -Xshare:dump objects would be the
wrong size. If CDS is not supported, then there should be something in
arguments.cpp that gives an error for that. And write a test for that error of
mixing and matching.
-
PR Review: https://git.openjdk.org/jdk/pull/13961#pullrequestreview-1424925882
PR Review Comment: https://git.openjdk.org/jdk/pull/13961#discussion_r1192648245
PR Review Comment: https://git.openjdk.org/jdk/pull/13961#discussion_r1192646637
PR Review Comment: https://git.openjdk.org/jdk/pull/13961#discussion_r1192661859
RFR: 8305895: Implementation: JEP 450: Compact Object Headers (Experimental)
This is the main body of the JEP 450: Compact Object Headers (Experimental). Main changes: - Introduction of the (experimental) flag UseCompactObjectHeaders. All changes in this PR are protected by this flag. - The compressed Klass* can now be stored in the mark-word of objects. In order to be able to do this, we are building on #10907, #13582 and #13779 to protect the relevant (upper 32) bits of the mark-word. Significant parts of this PR deal with loading the compressed Klass* from the mark-word, and dealing with (monitor-)locked objects. When the object is monitor-locked, we load the displaced mark-word from the monitor, and load the compressed Klass* from there. This PR also changes some code paths (mostly in GCs) to be more careful when accessing Klass* (or mark-word or size) to be able to fetch it from the forwardee in case the object is forwarded, and/or reach through to the monitor when the object is locked by a monitor. - The identity hash-code is narrowed to 25 bits. - Instances can now have their base-offset (the offset where the field layouter starts to place fields) at offset 8 (instead of 12 or 16). - Arrays will can now store their length at offset 8. Due to alignment restrictions, array elements will still start at offset 16. #11044 will resolve that restriction and allow array elements to start at offset 12 (except for long, double and uncompressed oops, which are still required to start at an element-aligned offset). - CDS can now write and read archives with the compressed header. However, it is not possible to read an archive that has been written with an opposite setting of UseCompactObjectHeaders. Testing: (+UseCompactObjectHeaders tests are run with the flag hard-patched into the build, to also catch @flagless tests, and to avoid mismatches with CDS - see above.) - [x] tier1 (x86_64) - [x] tier2 (x86_64) - [x] tier3 (x86_64) - [ ] tier4 (x86_64) - [x] tier1 (aarch64) - [x] tier2 (aarch64) - [x] tier3 (aarch64) - [ ] tier4 (aarch64) - [ ] tier1 (x86_64) +UseCompactObjectHeaders - [ ] tier2 (x86_64) +UseCompactObjectHeaders - [ ] tier3 (x86_64) +UseCompactObjectHeaders - [ ] tier4 (x86_64) +UseCompactObjectHeaders - [ ] tier1 (aarch64) +UseCompactObjectHeaders - [ ] tier2 (aarch64) +UseCompactObjectHeaders - [ ] tier3 (aarch64) +UseCompactObjectHeaders - [ ] tier4 (aarch64) +UseCompactObjectHeaders - Depends on: https://git.openjdk.org/jdk/pull/13779 Commit messages: - Consolidate _LP64 #ifdef - Remove obsolete check - Handle klass offset in JVMCI - Disable CDS tests when running with +UseCompactObjectHeaders - Merge branch 'JDK-8305898' into JDK-8305895-v2 - @colenp review comments - Remove obsolete code - Some hashcode improvements (mostly SA) - Merge remote-tracking branch 'origin/JDK-8305895' into JDK-8305895 - Fix some uses of klass_offset_in_bytes() - ... and 36 more: https://git.openjdk.org/jdk/compare/880d564a...a6e9f10a Changes: https://git.openjdk.org/jdk/pull/13961/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13961=00 Issue: https://bugs.openjdk.org/browse/JDK-8305895 Stats: 1306 lines in 98 files changed: 1025 ins; 94 del; 187 mod Patch: https://git.openjdk.org/jdk/pull/13961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13961/head:pull/13961 PR: https://git.openjdk.org/jdk/pull/13961
Re: RFR: 8308016: Use snippets in java.io package [v2]
> Replace `{@code ...}` patterns and the like with `{@snippet
> lang=java : ...}`.
Brian Burkhalter has updated the pull request incrementally with one additional
commit since the last revision:
8308016: Remove ellipses ("...") from snippets
-
Changes:
- all: https://git.openjdk.org/jdk/pull/13957/files
- new: https://git.openjdk.org/jdk/pull/13957/files/8a555a76..bc5c0358
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=13957=01
- incr: https://webrevs.openjdk.org/?repo=jdk=13957=00-01
Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/13957.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13957/head:pull/13957
PR: https://git.openjdk.org/jdk/pull/13957
Re: RFR: 8299340: CreateProcessW lpCommandLine must be mutable
On Fri, 12 May 2023 17:56:08 GMT, Naoto Sato wrote: >> Launching of processes on Windows using `ProcessCreateW` with a Unicode >> character set requires the buffer to be writable. An access violation might >> occur if `ProcessCreateW` writes to the command line string. The current >> implementation fetches the command line string using JNI GetStringChars >> returning a buffer that should not be modified. The code is unchanged since >> 2015. There have not been any reported faults in that time. >> >> This change copies the command line to a separately allocation mutable >> buffer to satisfy the Windows requirement. > > src/java.base/windows/native/libjava/ProcessImpl_md.c line 385: > >> 383: // Copy command line to mutable char buffer; >> CreateProcessW may modify it >> 384: jsize cmdLen = (*env)->GetStringLength(env, >> cmd); >> 385: WCHAR *pcmdCopy = (WCHAR*)malloc(cmdLen * >> sizeof(WCHAR)); > > Should this include null terminator, as it is interpreted as `LPWSTR` which > is null-terminated? It seems like it should now. Java strings are not null terminated and any null termination would have accidental. But it has worked for a long time (2013) without explicit Unicode null termination. - PR Review Comment: https://git.openjdk.org/jdk/pull/13894#discussion_r1192685105
Re: RFR: 8308021: Update IANA Language Subtag Registry to Version 2023-05-11
On Fri, 12 May 2023 17:56:12 GMT, Justin Lu wrote: > Please review this trivial fix which updates the IANA data to version > 5/11/2023. As the update only includes variant sub-tags, there is no impact > to JDK tests. The update can be found > [here](https://mm.icann.org/pipermail/ietf-languages-announcements/2023-May/87.html). Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13962#pullrequestreview-1424976281
Re: RFR: 8308021: Update IANA Language Subtag Registry to Version 2023-05-11
On Fri, 12 May 2023 17:56:12 GMT, Justin Lu wrote: > Please review this trivial fix which updates the IANA data to version > 5/11/2023. As the update only includes variant sub-tags, there is no impact > to JDK tests. The update can be found > [here](https://mm.icann.org/pipermail/ietf-languages-announcements/2023-May/87.html). Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13962#pullrequestreview-1424952709
Re: RFR: 8308016: Use snippets in java.io package
On Fri, 12 May 2023 17:52:29 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/Console.java line 124:
>>
>>> 122: * if (con != null) {
>>> 123: * Scanner sc = new Scanner(con.reader());
>>> 124: * ...
>>
>> I'm not sure how you feel about this, but when I have been converting
>> `` blocks to `@snippet`, I usually try to get rid of non code. Since
>> snippet allows user's to copy the example code, I feel like it's redudant to
>> leave in non code, since they will likely delete it anyways.
>
> I have been just trying to keep it as similar visually as possible but what
> you say is reasonable.
Agree, that I would make the code valid when using a snippet
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1192658575
RFR: 8308021: Update IANA Language Subtag Registry to Version 2023-05-11
Please review this trivial fix which updates the IANA data to version 5/11/2023. As the update only includes variant sub-tags, there is no impact to JDK tests. The update can be found [here](https://mm.icann.org/pipermail/ietf-languages-announcements/2023-May/87.html). - Commit messages: - Update LSRTest.java - Update registry.txt Changes: https://git.openjdk.org/jdk/pull/13962/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13962=00 Issue: https://bugs.openjdk.org/browse/JDK-8308021 Stats: 20 lines in 2 files changed: 17 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13962.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13962/head:pull/13962 PR: https://git.openjdk.org/jdk/pull/13962
Re: RFR: 8299340: CreateProcessW lpCommandLine must be mutable
On Tue, 9 May 2023 21:46:51 GMT, Roger Riggs wrote: > Launching of processes on Windows using `ProcessCreateW` with a Unicode > character set requires the buffer to be writable. An access violation might > occur if `ProcessCreateW` writes to the command line string. The current > implementation fetches the command line string using JNI GetStringChars > returning a buffer that should not be modified. The code is unchanged since > 2015. There have not been any reported faults in that time. > > This change copies the command line to a separately allocation mutable buffer > to satisfy the Windows requirement. src/java.base/windows/native/libjava/ProcessImpl_md.c line 385: > 383: // Copy command line to mutable char buffer; > CreateProcessW may modify it > 384: jsize cmdLen = (*env)->GetStringLength(env, cmd); > 385: WCHAR *pcmdCopy = (WCHAR*)malloc(cmdLen * > sizeof(WCHAR)); Should this include null terminator, as it is interpreted as `LPWSTR` which is null-terminated? - PR Review Comment: https://git.openjdk.org/jdk/pull/13894#discussion_r1192655747
Re: RFR: 8308016: Use snippets in java.io package
On Fri, 12 May 2023 17:31:32 GMT, Justin Lu wrote:
>> Replace `{@code ...}` patterns and the like with `{@snippet
>> lang=java : ...}`.
>
> src/java.base/share/classes/java/io/Console.java line 124:
>
>> 122: * if (con != null) {
>> 123: * Scanner sc = new Scanner(con.reader());
>> 124: * ...
>
> I'm not sure how you feel about this, but when I have been converting ``
> blocks to `@snippet`, I usually try to get rid of non code. Since snippet
> allows user's to copy the example code, I feel like it's redudant to leave in
> non code, since they will likely delete it anyways.
I have been just trying to keep it as similar visually as possible but what you
say is reasonable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1192652789
Re: RFR: 8307547: Support variant collations [v4]
On Thu, 11 May 2023 20:51:37 GMT, Naoto Sato wrote: >> The fix to https://bugs.openjdk.org/browse/JDK-8306927 switched the default >> collation for Swedish to the modern one. In order to provide a means for >> users who need the old collation, this PR intends to make `Collator` >> recognize the `co` Unicode locale extension so that multiple implementations >> for a locale can be provided. I would also like reviews for the >> corresponding CSR. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added "reformed" tests Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/13917#pullrequestreview-1424917959
Re: RFR: 8306597: Improve string formatting in EquivMapsGenerator.java [v4]
On Fri, 12 May 2023 17:25:45 GMT, Justin Lu wrote: >> Please review changes to `EquivMapsGenerator.java` (which is used to >> generate the Locale equivalencies for the JDK). >> >> The file previously used large concatenated Strings, which are now replaced >> with text blocks, in addition to some cleanup. No functionality is changed, >> the Locale equivalencies builds the same. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Review: capitalize and move footer text. Add comments to main Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13935#pullrequestreview-1424907858
RFR: 8299340: CreateProcessW lpCommandLine must be mutable
Launching of processes on Windows using `ProcessCreateW` with a Unicode character set requires the buffer to be writable. An access violation might occur if `ProcessCreateW` writes to the command line string. The current implementation fetches the command line string using JNI GetStringChars returning a buffer that should not be modified. The code is unchanged since 2015. There have not been any reported faults in that time. This change copies the command line to a separately allocation mutable buffer to satisfy the Windows requirement. - Commit messages: - 8299340: CreateProcessW lpCommandLine must be mutable Changes: https://git.openjdk.org/jdk/pull/13894/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13894=00 Issue: https://bugs.openjdk.org/browse/JDK-8299340 Stats: 17 lines in 1 file changed: 9 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/13894.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13894/head:pull/13894 PR: https://git.openjdk.org/jdk/pull/13894
Re: RFR: 8308016: Use snippets in java.io package
On Fri, 12 May 2023 16:17:38 GMT, Brian Burkhalter wrote:
> Replace `{@code ...}` patterns and the like with `{@snippet
> lang=java : ...}`.
src/java.base/share/classes/java/io/Console.java line 124:
> 122: * if (con != null) {
> 123: * Scanner sc = new Scanner(con.reader());
> 124: * ...
I'm not sure how you feel about this, but when I have been converting ``
blocks to `@snippet`, I usually try to get rid of non code. Since snippet
allows user's to copy the example code, I feel like it's redudant to leave in
non code, since they will likely delete it anyways.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1192635740
Re: RFR: 8306597: Improve string formatting in EquivMapsGenerator.java [v3]
On Fri, 12 May 2023 16:41:18 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove more ws in text block > > make/jdk/src/classes/build/tools/generatelsrequivmaps/EquivMapsGenerator.java > line 344: > >> 342: } >> 343: >> 344: private static final String footerText = "}\n\n}"; > > I'd prefer this one also be uppercased (and moved to a more suitable > location). ah, missed that one, probably because of it's location too. Made uppercased and moved closer to the rest of the static text variables. - PR Review Comment: https://git.openjdk.org/jdk/pull/13935#discussion_r1192626527
Re: RFR: 8306597: Improve string formatting in EquivMapsGenerator.java [v4]
> Please review changes to `EquivMapsGenerator.java` (which is used to generate > the Locale equivalencies for the JDK). > > The file previously used large concatenated Strings, which are now replaced > with text blocks, in addition to some cleanup. No functionality is changed, > the Locale equivalencies builds the same. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Review: capitalize and move footer text. Add comments to main - Changes: - all: https://git.openjdk.org/jdk/pull/13935/files - new: https://git.openjdk.org/jdk/pull/13935/files/33fd7993..b4e6e683 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13935=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13935=02-03 Stats: 13 lines in 1 file changed: 10 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13935.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13935/head:pull/13935 PR: https://git.openjdk.org/jdk/pull/13935
Re: RFR: 8307547: Support variant collations [v4]
On Fri, 12 May 2023 16:41:20 GMT, Steven Loomis wrote: >>> I'm just wondering where a developer might go to get a definitive list, >>> i.e. aside from this API note, how would they know that "-trad" or >>> "-traditional" can be used to configure the ordering. Locale.forLanguageTag >>> supports more than BCP 47 language tag strings so is this considered a >>> private use language tag. >> >> I think those should go into Oracle JDK's `Supported Locales` document. >> Created a task to include them (https://bugs.openjdk.org/browse/JDK-8308018) > >> > I'm just wondering where a developer might go to get a definitive list, >> > i.e. aside from this API note, how would they know that "-trad" or >> > "-traditional" can be used to configure the ordering. >> > Locale.forLanguageTag supports more than BCP 47 language tag strings so is >> > this considered a private use language tag. >> >> I think those should go into Oracle JDK's `Supported Locales` document. >> Created a task to include them (https://bugs.openjdk.org/browse/JDK-8308018) > > `-u-co` is defined > [here](https://www.unicode.org/reports/tr35/#UnicodeCollationIdentifier) > (UTS#35) and the keys are in the datafile [here (link to `main` > branch!)](https://github.com/unicode-org/cldr/blob/main/common/bcp47/collation.xml) > - note the `since=` attribute, these are very stable. > > @AlanBateman >> Locale.forLanguageTag supports more than BCP 47 language tag strings > > It should still be all valid BCP47 including extensions and private use (such > as x-lvalue). > >> so is this considered a private use language tag > > Not private use at all. The `-u-` subtag is registered, and the links above > are from the registrar, see > - https://www.rfc-editor.org/info/bcp47 > - https://www.rfc-editor.org/rfc/rfc6067 > Thanks @srl295 > I am just curious how CLDR handles the default switch of Swedish collation. > Now the `traditional` collation used to be `standard`, and `standard` used to > be `reformed`. How do apps specify their desired collation in Swedish, > regardless of CLDR versions? I don't know off the top of my head but digging around I found https://unicode-org.atlassian.net/browse/CLDR-7088 and cross linked some tickets. Can you ask around on those? - PR Comment: https://git.openjdk.org/jdk/pull/13917#issuecomment-1546050297
Re: RFR: 8307547: Support variant collations [v4]
On Fri, 12 May 2023 16:41:20 GMT, Steven Loomis wrote: >>> I'm just wondering where a developer might go to get a definitive list, >>> i.e. aside from this API note, how would they know that "-trad" or >>> "-traditional" can be used to configure the ordering. Locale.forLanguageTag >>> supports more than BCP 47 language tag strings so is this considered a >>> private use language tag. >> >> I think those should go into Oracle JDK's `Supported Locales` document. >> Created a task to include them (https://bugs.openjdk.org/browse/JDK-8308018) > >> > I'm just wondering where a developer might go to get a definitive list, >> > i.e. aside from this API note, how would they know that "-trad" or >> > "-traditional" can be used to configure the ordering. >> > Locale.forLanguageTag supports more than BCP 47 language tag strings so is >> > this considered a private use language tag. >> >> I think those should go into Oracle JDK's `Supported Locales` document. >> Created a task to include them (https://bugs.openjdk.org/browse/JDK-8308018) > > `-u-co` is defined > [here](https://www.unicode.org/reports/tr35/#UnicodeCollationIdentifier) > (UTS#35) and the keys are in the datafile [here (link to `main` > branch!)](https://github.com/unicode-org/cldr/blob/main/common/bcp47/collation.xml) > - note the `since=` attribute, these are very stable. > > @AlanBateman >> Locale.forLanguageTag supports more than BCP 47 language tag strings > > It should still be all valid BCP47 including extensions and private use (such > as x-lvalue). > >> so is this considered a private use language tag > > Not private use at all. The `-u-` subtag is registered, and the links above > are from the registrar, see > - https://www.rfc-editor.org/info/bcp47 > - https://www.rfc-editor.org/rfc/rfc6067 Thanks @srl295 I am just curious how CLDR handles the default switch of Swedish collation. Now the `traditional` collation used to be `standard`, and `standard` used to be `reformed`. How do apps specify their desired collation in Swedish, regardless of CLDR versions? - PR Comment: https://git.openjdk.org/jdk/pull/13917#issuecomment-1546042805
Re: RFR: 8300794: Use @snippet in java.util:i18n [v2]
On Thu, 11 May 2023 20:39:57 GMT, Justin Lu wrote: >> Please review this javadoc only change which uses `@snippet` and >> `@linkplain` in i18n related util packages. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Review: Move links on top, fix indentation in example Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13920#pullrequestreview-1424868788
Re: RFR: 8306597: Improve string formatting in EquivMapsGenerator.java [v3]
On Thu, 11 May 2023 22:23:47 GMT, Justin Lu wrote: >> Please review changes to `EquivMapsGenerator.java` (which is used to >> generate the Locale equivalencies for the JDK). >> >> The file previously used large concatenated Strings, which are now replaced >> with text blocks, in addition to some cleanup. No functionality is changed, >> the Locale equivalencies builds the same. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Remove more ws in text block make/jdk/src/classes/build/tools/generatelsrequivmaps/EquivMapsGenerator.java line 344: > 342: } > 343: > 344: private static final String footerText = "}\n\n}"; I'd prefer this one also be uppercased (and moved to a more suitable location). - PR Review Comment: https://git.openjdk.org/jdk/pull/13935#discussion_r1192593829
Re: RFR: 8307547: Support variant collations [v4]
On Fri, 12 May 2023 16:32:57 GMT, Naoto Sato wrote: > > I'm just wondering where a developer might go to get a definitive list, > > i.e. aside from this API note, how would they know that "-trad" or > > "-traditional" can be used to configure the ordering. Locale.forLanguageTag > > supports more than BCP 47 language tag strings so is this considered a > > private use language tag. > > I think those should go into Oracle JDK's `Supported Locales` document. > Created a task to include them (https://bugs.openjdk.org/browse/JDK-8308018) `-u-co` is defined [here](https://www.unicode.org/reports/tr35/#UnicodeCollationIdentifier) (UTS#35) and the keys are in the datafile [here (link to `main` branch!)](https://github.com/unicode-org/cldr/blob/main/common/bcp47/collation.xml) - note the `since=` attribute, these are very stable. - PR Comment: https://git.openjdk.org/jdk/pull/13917#issuecomment-1546013166
Re: RFR: 8307547: Support variant collations [v4]
On Thu, 11 May 2023 20:51:37 GMT, Naoto Sato wrote: >> The fix to https://bugs.openjdk.org/browse/JDK-8306927 switched the default >> collation for Swedish to the modern one. In order to provide a means for >> users who need the old collation, this PR intends to make `Collator` >> recognize the `co` Unicode locale extension so that multiple implementations >> for a locale can be provided. I would also like reviews for the >> corresponding CSR. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added "reformed" tests Marked as reviewed by srl (Committer). - PR Review: https://git.openjdk.org/jdk/pull/13917#pullrequestreview-1424842005
Re: RFR: 8300794: Use @snippet in java.util:i18n [v2]
On Thu, 11 May 2023 20:39:57 GMT, Justin Lu wrote: >> Please review this javadoc only change which uses `@snippet` and >> `@linkplain` in i18n related util packages. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Review: Move links on top, fix indentation in example Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13920#pullrequestreview-1424841206
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]
On Tue, 17 Jan 2023 15:55:40 GMT, Jan Lahoda wrote:
>> The pattern matching switches are using a bootstrap method
>> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch.
>> Basically, for a switch like:
>>
>> switch (obj) {
>> case String s when s.isEmpty() -> {}
>> case String s -> {}
>> case CharSequence cs -> {}
>> ...
>> }
>>
>>
>> this method will produce a MethodHandle that will be analyze the provided
>> selector value (`obj` in the example), and will return the case index to
>> which the switch should jump. This method also accepts a (re)start index for
>> the search, which is used to handle guards. For example, if the
>> `s.isEmpty()` guard in the above sample returns false, the matching is
>> restarted on the next case.
>>
>> The current implementation is fairly slow, it basically goes through the
>> labels in a loop. The proposal here is to replace that with a MethodHandle
>> structure like this:
>>
>> obj == null ? -1
>> : switch (restartIndex) {
>> case 0 -> obj instanceof String ? 0 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 1 -> obj instanceof String ? 1 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 2 -> obj instanceof CharSequence ? 2 : ... ;
>> ...
>> default -> ;
>> }
>>
>>
>> This appear to run faster than the current implementation, using testcase
>> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these
>> are the results
>>
>> PatternsOptimizationTest.testLegacyIndyLongSwitch thrpt 25 1515989.562
>> ± 32047.918 ops/s
>> PatternsOptimizationTest.testHandleIndyLongSwitch thrpt 25 2630707.585
>> ± 37202.210 ops/s
>>
>> PatternsOptimizationTest.testLegacyIndyShortSwitch thrpt 25 6789310.900
>> ± 61921.636 ops/s
>> PatternsOptimizationTest.testHandleIndyShortSwitch thrpt 25 10771729.464
>> ± 69607.467 ops/s
>>
>>
>> The "LegacyIndy" is the current implementation, "HandleIndy" is the one
>> proposed here. The translation in javac used is the one from #9746 in all
>> cases.
>
> Jan Lahoda 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 four additional commits since
> the last revision:
>
> - Adding comments
> - Improving performance
> - Merge branch 'master' into JDK-8291966
> - 8291966: SwitchBootstrap.typeSwitch could be faster
I did several experiments with generated lookupswitch over class hashes (for
final and sealed class labels only).
My experiments and benchmark results are actually showing following facts:
- traversal expanded to tableswitch (as proposed here) is significantly faster
than circulating over an array
- expanded lookupswitch over class hashes is unfortunately slower than this
proposal, even when benchmarked on large switches containing final or sealed
class labels only
-
Marked as reviewed by asotona (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/9779#pullrequestreview-1424836407
Re: RFR: 8307547: Support variant collations [v4]
On Fri, 12 May 2023 08:45:37 GMT, Alan Bateman wrote: > I'm just wondering where a developer might go to get a definitive list, i.e. > aside from this API note, how would they know that "-trad" or "-traditional" > can be used to configure the ordering. Locale.forLanguageTag supports more > than BCP 47 language tag strings so is this considered a private use language > tag. I think those should go into Oracle JDK's `Supported Locales` document. Created a task to include them (https://bugs.openjdk.org/browse/JDK-8308018) - PR Comment: https://git.openjdk.org/jdk/pull/13917#issuecomment-1546003990
RFR: 8308016: Use snippets in java.io package
Replace `{@code ...}` patterns and the like with `{@snippet
lang=java : ...>`.
-
Commit messages:
- 8308016: Use snippets in java.io package
Changes: https://git.openjdk.org/jdk/pull/13957/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=13957=00
Issue: https://bugs.openjdk.org/browse/JDK-8308016
Stats: 178 lines in 21 files changed: 27 ins; 7 del; 144 mod
Patch: https://git.openjdk.org/jdk/pull/13957.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13957/head:pull/13957
PR: https://git.openjdk.org/jdk/pull/13957
Integrated: 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph
On Thu, 11 May 2023 10:00:39 GMT, Maurizio Cimadamore wrote: > As the title says, this patch fixes an issue with > `MemorySegment::reinterpet`, which reports the customary "restricted method" > section twice. > > I also fixed some wrong capitalization in the text of the factories in > `Linker.Option`. This pull request has now been integrated. Changeset: 6ebea897 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/6ebea8973feb08a7443d8d86ff52f453dc4aec43 Stats: 8 lines in 2 files changed: 0 ins; 5 del; 3 mod 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13926
RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
Since JDK13, executing commands in a sub-process defaults to the so called
`POSIX_SPAWN` launching mechanism (i.e.
`-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by using
`posix_spawn(3)` to firstly start a tiny helper program called `jspawnhelper`
in a subprocess. In a second step, `jspawnhelper` reads the command data from
the parent Java process over a Unix pipe and finally executes (i.e.
`execvp(3)`) the requested command.
In cases where the parent process terminates abnormally before `jspawnhelper`
has read all the expected data from the pipe, `jspawnhelper` will block
indefinitely on the reading end of the pipe. This is especially harmful if the
parent process had open sockets, because in that case, the forked
`jspawnhelper` process will inherit them and keep all the corresponding ports
open effectively preventing other processes to bind to them. Notice that this
is not an abstract scenario. We've observed this regularly in production with
services which couldn't be restarted after a crash after migrating to JDK 17.
The fix of the issue is rather trivial. `jspawnhelper` has to close its writing
end of the pipe which connects it with the parent Java process *before*
starting to read from that pipe such that reads from the pipe can immediately
return with EOF if the parent process terminates abnormally.
Also did some cleanup:
- in `jspawnhelper.c` correctly chek the result of `readFully()`
- in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to write
the command data from the parent process to `jspawnhelper`
- in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because that's
long gone.
-
Commit messages:
- 8307990: jspawnhelper must close its writing side of a pipe before reading
from it
Changes: https://git.openjdk.org/jdk/pull/13956/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=13956=00
Issue: https://bugs.openjdk.org/browse/JDK-8307990
Stats: 29 lines in 4 files changed: 9 ins; 1 del; 19 mod
Patch: https://git.openjdk.org/jdk/pull/13956.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13956/head:pull/13956
PR: https://git.openjdk.org/jdk/pull/13956
Integrated: 8307535: java.util.logging.Handlers should be more VirtualThread friendly
On Fri, 5 May 2023 13:43:43 GMT, Daniel Fuchs wrote: > Several Handlers class use monitors to synchronize when formatting / > publishing LogRecords. > When logging within a VirtualThread, holding this monitor can cause the > carrier thread to be pinned. > Handlers could use jdk.internal.misc.InternalLock, in a similar way to some > java.io classes (such as PrintStream) to avoid pinning the carrier thread. This pull request has now been integrated. Changeset: 3c68c352 Author:Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/3c68c352fc3d3bff3d80bafcf04118759f4a2acf Stats: 309 lines in 6 files changed: 276 ins; 2 del; 31 mod 8307535: java.util.logging.Handlers should be more VirtualThread friendly Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/13832
Re: RFR: 8306842: Classfile API performance improvements [v4]
On Wed, 10 May 2023 10:00:47 GMT, Adam Sotona wrote:
>> Following improvements implemented:
>> - Switch over `String` replaced with switch single char
>> - Binary search for frames in `StackMapGenerator`
>> - `StackMapGenerator.rawHandlers` with pre-calculated offsets
>> - `ClassEntry` is caching `ClassDesc` symbol
>> - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry`
>> - Caching `MethodTypeDesc` in `MethodInfo` implementations
>> - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc`
>> instead of custom parsing
>>
>> No API change.
>>
>> Benchmarks show stack map generation improved by 21% and code generation
>> from symbols improved by 30%.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>> GenerateStackMaps.benchmark thrpt 10 407931.202 ± 13101.023 ops/s
>> RebuildMethodBodies.shared thrpt4 10258.597 ± 383.699 ops/s
>> RebuildMethodBodies.unshared thrpt47224.543 ± 256.800 ops/s
>>
>>
>>
>> Benchmark Mode Cnt Score Error Units
>> GenerateStackMaps.benchmark thrpt 10 495101.110 ± 2389.628 ops/s
>> RebuildMethodBodies.sharedthrpt 4 13380.272 ± 810.113 ops/s
>> RebuildMethodBodies.unshared thrpt 49399.863 ± 557.060 ops/s
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed jmh benchmark parameters
test/micro/org/openjdk/bench/jdk/classfile/RebuildMethodBodies.java line 57:
> 55: public void setup() throws IOException {
> 56: shared = new LinkedList<>();
> 57: unshared = new LinkedList<>();
LinkedList should be replaced by ArrayList, as:
1. LinkedList's node wrapper around each object is too heavy, ArrayList has
smaller wrapper
2. LinkedList iteration needs to follow links while ArrayList access can be
machine optimized
3. ArrayList addition is amortized O(1), not really worse than LinkedList
additions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13671#discussion_r1192367198
Re: RFR: 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph
On Thu, 11 May 2023 10:00:39 GMT, Maurizio Cimadamore wrote: > As the title says, this patch fixes an issue with > `MemorySegment::reinterpet`, which reports the customary "restricted method" > section twice. > > I also fixed some wrong capitalization in the text of the factories in > `Linker.Option`. Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13926#pullrequestreview-1424490386
Re: RFR: 8307535: java.util.logging.Handlers should be more VirtualThread friendly [v4]
On Thu, 11 May 2023 18:34:45 GMT, Daniel Fuchs wrote: >> Several Handlers class use monitors to synchronize when formatting / >> publishing LogRecords. >> When logging within a VirtualThread, holding this monitor can cause the >> carrier thread to be pinned. >> Handlers could use jdk.internal.misc.InternalLock, in a similar way to some >> java.io classes (such as PrintStream) to avoid pinning the carrier thread. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Use ReentrantLock directly Marked as reviewed by jpai (Reviewer). The latest changes to use `ReentrantLock` instead of `InternalLock` looks fine to me. - PR Review: https://git.openjdk.org/jdk/pull/13832#pullrequestreview-1424455046 PR Comment: https://git.openjdk.org/jdk/pull/13832#issuecomment-1545699479
RFR: 8307403: java/util/zip/DeInflate.java timed out
Can I please get a review of this test only change which addresses the issue noted in https://bugs.openjdk.org/browse/JDK-8307403? When we recently did a change in https://bugs.openjdk.org/browse/JDK-8299748, there was a oversight that the `deflate.deflate(tempBuffer)` could potentially end up taking in a zero sized array. That would then mean the loop in which this `deflate` happens, would never exit leading to the test timing out. I've added additional details as a comment to the JBS issue https://bugs.openjdk.org/browse/JDK-8307403?focusedCommentId=14581202=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14581202. The commit in this PR uses a fixed size temporary buffer, unrelated to the input length, to get past this problem. The use of fixed size here will not re-introduce the original issue which https://bugs.openjdk.org/browse/JDK-8299748 fixed, because unlike previously, this is just a temporary size and we continue to deflate the content into a dynamically growing `ByteArrayOutputStream` till the deflater is "finished". Additionally I've added a log message to help debug any future issues in this call. - Commit messages: - 8307403: java/util/zip/DeInflate.java timed out Changes: https://git.openjdk.org/jdk/pull/13955/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13955=00 Issue: https://bugs.openjdk.org/browse/JDK-8307403 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13955.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13955/head:pull/13955 PR: https://git.openjdk.org/jdk/pull/13955
RFR: 8287834: Add SymbolLookup::or method
This patch adds a simpler method for composing symbol lookups. It is common for clients to chain multiple symbol lookups together, e.g. to find a symbol in multiple libraries. A new instance method, namely `SymbolLookup::or` is added, which first searches a symbol in the first lookup, and, if that fails, proceeds to search the symbol in the provided lookup. We have considered alternatives to express this, such as a static factory `SymbolLookup::ofComposite` but settled on this because of the similarity with `Optional::or`. - Commit messages: - Add test for composite lookups - Initial push Changes: https://git.openjdk.org/jdk/pull/13954/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13954=00 Issue: https://bugs.openjdk.org/browse/JDK-8287834 Stats: 169 lines in 2 files changed: 169 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13954.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13954/head:pull/13954 PR: https://git.openjdk.org/jdk/pull/13954
Integrated: 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere
On Thu, 11 May 2023 21:29:51 GMT, Maurizio Cimadamore wrote: > This patch fixes the JNI test for the enable native access flag, which was > updated incorrectly as part of https://git.openjdk.org/jdk/pull/13863. > > The problem is that the test doesn't make global references out of the local > ones before sharing them with another thread. > > I could reproduce the failure locally with `-Xcheck:jni`. With this patch, > the test passes. This pull request has now been integrated. Changeset: 13a3fce2 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/13a3fce29e696354b2e79fbcfd3557dc4a1fece7 Stats: 8 lines in 1 file changed: 3 ins; 0 del; 5 mod 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13944
Re: RFR: 8307547: Support variant collations [v4]
On Thu, 11 May 2023 20:51:37 GMT, Naoto Sato wrote: >> The fix to https://bugs.openjdk.org/browse/JDK-8306927 switched the default >> collation for Swedish to the modern one. In order to provide a means for >> users who need the old collation, this PR intends to make `Collator` >> recognize the `co` Unicode locale extension so that multiple implementations >> for a locale can be provided. I would also like reviews for the >> corresponding CSR. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added "reformed" tests The update, and API note in Collator.getInstance(Locale), looks okay to me. I'm just wondering where a developer might go to get a definitive list, i.e. aside from this API note, how would they know that "-trad" or "-traditional" can be used to configure the ordering. Locale.forLanguageTag supports more than BCP 47 language tag strings so is this considered a private use language tag. - PR Comment: https://git.openjdk.org/jdk/pull/13917#issuecomment-1545395782
