Re: RFR: 8331222: Malformed text in the jpackage doc page
On Tue, 30 Apr 2024 23:58:49 GMT, Alexey Semenyuk wrote: > Rerun pandoc on updated source man page file Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19028#pullrequestreview-2032839569
RFR: 8331222: Malformed text in the jpackage doc page
Rerun pandoc on updated source man page file - Commit messages: - 8331222: Malformed text in the jpackage doc page Changes: https://git.openjdk.org/jdk/pull/19028/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19028&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331222 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19028.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19028/head:pull/19028 PR: https://git.openjdk.org/jdk/pull/19028
Re: RFR: 8331320: ClassFile API OutOfMemoryError with certain class files
On Tue, 30 Apr 2024 18:18:30 GMT, Paul Sandoz wrote:
>> Class files with specifically corrupted tableswitch or lookupswitch
>> instructions in the bytecode cause OutOfMemoryError while parsing with
>> Class-File API.
>> This patch performs additional checks to avoid OOME and adds relevant tests.
>>
>> Please review.
>>
>> Thank you,
>> Adam
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
> line 320:
>
>> 318: int low = code.classReader.readInt(ap + 4);
>> 319: int high = code.classReader.readInt(ap + 8);
>> 320: if (high < low || high - low > code.codeLength >> 2) {
>
> May be its also an opportunity to reduce duplication e.g., replace line 316
> with a call to `afterPadding()`
`BoundTableSwitchInstruction::afterPadding()` is an instance method, and
`BoundTableSwitchInstruction::size(…)` is a static method, so this would
require further refactoring.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19024#discussion_r1585700103
Re: RFR: 8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Sun, 21 Apr 2024 04:37:45 GMT, Srinivas Vamsi Parasa wrote: >> Hello Vamsi (@vamsi-parasa), >> >> Could you please run the new benchmarking? >> To save time and don't patch JDK several times, I've created >> JavaBenchmarkHarness >> class which is under package java.util and compares several versions of DPQS. >> Also I prepared several versions of current sorting source from JDK to >> detect what is going wrong. >> >> What you need is to compile and run JavaBenchmarkHarness once: >> >> javac --patch-module java.base=. -d classes *.java >> java -XX:CompileThreshold=1 -XX:-TieredCompilation --patch-module >> java.base=classes -cp classes java.util.JavaBenchmarkHarness >> >> Find the sources there: >> >> https://github.com/iaroslavski/sorting/blob/master/radixsort/JavaBenchmarkHarness.java >> >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_ins.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_mrg.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_piv.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_prt.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r29p.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r29p5.java >> >> Thank you, >> Vladimir > > Hi Vladimir (@iaroslavski), > > Please see the data below: > > Thanks, > Vamsi > > > > name | builder | size | mode | count | score > -- | -- | -- | -- | -- | -- > b01 | RANDOM | 600 | avg | 325677 | 6.862 > b01 | RANDOM | 3000 | avg | 52041 | 82.233 > b01 | RANDOM | 9 | avg | 1217 | 4456.51 > b01 | RANDOM | 40 | avg | 242 | 22923.28 > b01 | RANDOM | 100 | avg | 90 | 60598.84 > b01 | REPEATED | 600 | avg | 651354 | 1.933 > b01 | REPEATED | 3000 | avg | 104083 | 13.753 > b01 | REPEATED | 9 | avg | 2435 | 723.039 > b01 | REPEATED | 40 | avg | 484 | 3084.416 > b01 | REPEATED | 100 | avg | 180 | 8234.428 > b01 | STAGGER | 600 | avg | 1954062 | 1.005 > b01 | STAGGER | 3000 | avg | 312251 | 4.945 > b01 | STAGGER | 9 | avg | 7305 | 133.126 > b01 | STAGGER | 40 | avg | 1453 | 592.144 > b01 | STAGGER | 100 | avg | 542 | 1493.876 > b01 | SHUFFLE | 600 | avg | 325677 | 5.12 > b01 | SHUFFLE | 3000 | avg | 52041 | 29.252 > b01 | SHUFFLE | 9 | avg | 1217 | 1396.664 > b01 | SHUFFLE | 40 | avg | 242 | 5743.489 > b01 | SHUFFLE | 100 | avg | 90 | 15490.81 > b01_ins | RANDOM | 600 | avg | 325677 | 7.594 > b01_ins | RANDOM | 3000 | avg | 52041 | 78.631 > b01_ins | RANDOM | 9 | avg | 1217 | 4312.511 > b01_ins | RANDOM | 40 | avg | 242 | 22108.18 > b01_ins | RANDOM | 100 | avg | 90 | 58467.16 > b01_ins | REPEATED | 600 | avg | 651354 | 1.569 > b01_ins | REPEATED | 3000 | avg | 104083 | 11.313 > b01_ins | REPEATED | 9 | avg | 2435 | 720.838 > b01_ins | REPEATED | 40 | avg | 484 | 3003.673 > b01_ins | REPEATED | 100 | avg | 180 | 8144.944 > b01_ins | STAGGER | 600 | avg | 1954062 | 0.98 > b01_ins | STAGGER | 3000 | avg | 312251 | 4.948 > b01_ins | STAGGER | 9 | avg | 7305 | 132.909 > b01_ins | STAGGER | 40 | avg | 1453 | 592.572 > b01_ins | STAGGER | 100 | avg | 542 | 1492.627 > b01_ins | SHUFFLE | 600 | avg | 325677 | 4.092 > b01_ins | SHUFFLE | 3000 | avg | 52041 | 27.138 > b01_ins | SHUFFLE | 9 | avg | 1217 | 1304.326 > b01_ins | SHUFFLE | 40 | avg | 242 | 5465.745 > b01_ins | SHUFFLE | 100 | avg | 90 | 14585.08 > b01_mrg | RANDOM | 600 | avg | 325677 | 7.139 > b01_mrg | RANDOM | 3000 | avg | 52041 | 81.01 > b01_mrg | RANDOM | 9 | avg | 1217 | 4266.084 > b01_mrg | RANDOM | 40 | avg | 242 | 21937.77 > b01_mrg | RANDOM | 100 | avg | 90 | 58177.72 > b01_mrg | REPEATED | 600 | avg | 651354 | 1.36 > b01_mrg | REPEATED | 3000 | avg | 104083 | 9.013 > b01_mrg | REPEATED | 9 | avg | 2435 | 737.684 > b01_mrg | REPEATED | 40 | avg | 484 | 3152.447 > b01_mrg | REPEATED | 100 | avg |... Hello Vamsi (@vamsi-parasa), Could you please run the new benchmarking to detect the best case for Radix sort and parallel sorting? What you need is to compile and run JavaBenchmarkHarness: javac --patch-module java.base=. -d classes *.java java -XX:CompileThreshold=1 -XX:-TieredCompilation --patch-module java.base=classes -cp classes java.util.JavaBenchmarkHarness Find the sources there: https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_a.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_11.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_12.java https://g
Re: RFR: 8278255: Add more warning text in ReentrantLock and ReentrantReadWriteLock [v2]
On Sat, 27 Apr 2024 11:52:18 GMT, Viktor Klang wrote: >> This is an attempt to be more clear about recommendations on Lock usage. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java > > Co-authored-by: Pavel Rappo <32523691+pavelra...@users.noreply.github.com> The grammar rules prefer "that" because it is a restrictive clause, even though "which" might sound better. See for example: https://owl.purdue.edu/owl/general_writing/grammar/that_vs_which.html - PR Comment: https://git.openjdk.org/jdk/pull/18974#issuecomment-2086883300
Re: RFR: 8331320: ClassFile API OutOfMemoryError with certain class files
On Tue, 30 Apr 2024 15:31:02 GMT, Adam Sotona wrote:
> Class files with specifically corrupted tableswitch or lookupswitch
> instructions in the bytecode cause OutOfMemoryError while parsing with
> Class-File API.
> This patch performs additional checks to avoid OOME and adds relevant tests.
>
> Please review.
>
> Thank you,
> Adam
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
line 320:
> 318: int low = code.classReader.readInt(ap + 4);
> 319: int high = code.classReader.readInt(ap + 8);
> 320: if (high < low || high - low > code.codeLength >> 2) {
May be its also an opportunity to reduce duplication e.g., replace line 316
with a call to `afterPadding()`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19024#discussion_r1585300727
Integrated: 8331207: Misleading example in DateFormat#parse docs
On Fri, 26 Apr 2024 17:47:46 GMT, Justin Lu wrote: > Correct a misleading example in the DateFormat parse documentation. > > Common usage is to use a factory instance. The original example provided > assumes a pattern provided by a COMPAT factory instance. > > The pattern itself should be explicitly provided, since users may be using > CLDR factory instances. This pull request has now been integrated. Changeset: a863ef5d Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/a863ef5d74e9001a143af4638422348ee946c939 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8331207: Misleading example in DateFormat#parse docs Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/18983
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Mon, 29 Apr 2024 21:53:02 GMT, Tim Prinzing wrote: > Should I set the default to be fairly high (like maybe 1600ms)? I think to be > useful people will have to set the threshold to something that fits their > needs anyway. I wonder about the usefulness of this event if the default threshold is so high. My guess is that it's rare to change the default threshold. To your question then I suppose this comes back to how this event might be used. Maybe it's worth writing down a few possible usages and that might help to inform if the threshold should be 20ms or something else. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1585104188
Integrated: 8329138: Convert JFR FileForceEvent to static mirror event
On Fri, 29 Mar 2024 00:52:46 GMT, Tim Prinzing wrote: > Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. This pull request has now been integrated. Changeset: f4caac8d Author:Tim Prinzing Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/f4caac8dea1c95234743712386cb28a1ecb11197 Stats: 280 lines in 12 files changed: 238 ins; 28 del; 14 mod 8329138: Convert JFR FileForceEvent to static mirror event Reviewed-by: alanb, egahlin - PR: https://git.openjdk.org/jdk/pull/18542
RFR: 8331320: ClassFile API OutOfMemoryError with certain class files
Class files with specifically corrupted tableswitch or lookupswitch instructions in the bytecode cause OutOfMemoryError while parsing with Class-File API. This patch performs additional checks to avoid OOME and adds relevant tests. Please review. Thank you, Adam - Commit messages: - 8331320: ClassFile API OutOfMemoryError with certain class files Changes: https://git.openjdk.org/jdk/pull/19024/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19024&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331320 Stats: 60 lines in 2 files changed: 59 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19024.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19024/head:pull/19024 PR: https://git.openjdk.org/jdk/pull/19024
Integrated: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
On Mon, 29 Apr 2024 16:42:05 GMT, Viktor Klang wrote: > This PR finalizes JEP 473 by modifying the PreviewFeature JEP number and > status for Stream Gatherers. This pull request has now been integrated. Changeset: 2cc8eccb Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/2cc8eccb360848f3ddf3259f1d943552f86234b9 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473 Reviewed-by: pminborg, alanb - PR: https://git.openjdk.org/jdk/pull/19003
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v3]
> Consider code like:
>
> public class ConsoleTest {
> public static void main(String... args) {
> System.console().printf("Hello!");
> }
> }
>
>
> When run as:
>
> $ java ConsoleTest.java >/dev/null
>
>
> it prints `Hello!` to stderr, instead of to stdout (where it would be
> redirected).
>
> The proposed fix is to simply force the use of stdout. Sadly, this cannot be
> done solely using JLine configuration, we actually need to change the JLine's
> code for that.
>
> The most tricky part is a test. There are two sub-tests, one effectively
> testing a case where all of stdin/out/err are redirected, the other is
> attempting to test the case where stdin is attached to a terminal, while
> stdout is redirected. The second sub-test using a native functions to create
> a pty and to attach to it, and should run in a separate VM, as it leaves the
> VM attached to the terminal.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Adjusting test, as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18996/files
- new: https://git.openjdk.org/jdk/pull/18996/files/8a918e3f..76599ac9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18996&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18996&range=01-02
Stats: 57 lines in 1 file changed: 6 ins; 35 del; 16 mod
Patch: https://git.openjdk.org/jdk/pull/18996.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18996/head:pull/18996
PR: https://git.openjdk.org/jdk/pull/18996
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v2]
On Tue, 30 Apr 2024 14:01:37 GMT, Jan Lahoda wrote: > I.e. not trying to be too smart about output, and simply using stdout as > other programs do seems consistent, and most useful - the output can then be > used in pipes, etc. Totally reasonable. But the ssh example is telling - suppose for example someone wanted to implement something like ssh in Java. Would that person expect to be able to do what ssh does (accepting non-echoed passwords) by using `System.console()` ? In other words, what exactly is `System.console()` supposed to represent? Maybe that question pinpoints the source of the ambiguity here. Of course, it will differ by O/S which adds to the challenge. These questions are probably beyond the scope of this PR but you've got me curious :) - PR Comment: https://git.openjdk.org/jdk/pull/18996#issuecomment-2085559962
RFR: 8331427: Rename confusingly named ArraysSupport.signedHashCode
Please review this trivial method rename. While this issue was originally spotted in [another PR], it makes sense to address it separately. Test results are pending; not that I expect failures, but still. [another PR]: https://github.com/openjdk/jdk/pull/14831#issuecomment-1655477396 - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/19023/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19023&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331427 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19023.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19023/head:pull/19023 PR: https://git.openjdk.org/jdk/pull/19023
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v2]
On Mon, 29 Apr 2024 20:21:23 GMT, Archie Cobbs wrote: > > Of course the question is if it should write to stderr or /dev/tty like > > mechanism.. > > I was wondering the same thing. My understanding of the definition of > "console" is a bidirectional byte channel with a keyboard & screen on the > other end. It has no concept of stdout vs. stderr. It just has a display (or > in the old days, a printer). > > The function of a "shell" is to intermediate between one or more executing > programs and the console. It figures out how & when to actually display on > the console any stuff written to stdout and/or stderr by one of the programs > it has launched. > > A program can also access its console (if any) directly by opening /dev/tty > or whatever, thereby bypassing the shell. > > So I would think writing to something called "System.console()" from Java > (which is a program) would have nothing to do with Java's stderr or stdout, > except for a possible downstream interleaving with what the shell may also be > writing to the console at the same time. I tried a few programs, like `mc`, `top` and `htop`. When I redirect the stdout for them, they still write into it (and there's obviously nothing on the terminal, and there are escape sequences in the target file of the redirect). The only program I know from the top of my head that (AFAIK) bypasses stdin/stdout and reaches directly to the controlling terminal is `ssh` when reading passwords (for quite obvious very special reasons). I.e. not trying to be too smart about output, and simply using stdout as other programs do seems consistent, and most useful - the output can then be used in pipes, etc. - PR Comment: https://git.openjdk.org/jdk/pull/18996#issuecomment-2085419986
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v3]
> Hi, > During performance optimization work on Class-File API as JDK lambda > generator we found some static initialization killers. > One of them is `java.lang.classfile.Attributes` with tens of static fields > initialized with individual attribute mappers, and common set of all mappers, > and static map from attribute names to the mappers. > > I propose to turn all the static fields into lazy-initialized static methods > and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` > static mapping method from the `Attributes` API class. > > Please let me know your comments or objections and please review the > [PR](https://github.com/openjdk/jdk/pull/19006) and > [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: changed order in allowed modules attributes check - Changes: - all: https://git.openjdk.org/jdk/pull/19006/files - new: https://git.openjdk.org/jdk/pull/19006/files/27238368..f0d9174e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19006&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19006&range=01-02 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19006.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19006/head:pull/19006 PR: https://git.openjdk.org/jdk/pull/19006
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Sun, 10 Mar 2024 14:40:09 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 35 commits: > > - Fix whitespace > - Merge branch 'master' into master-cgroup > >Conflicts: > test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp > - Fix gtest > - Update the Java part > - Fix cgroup1 backward compatibility message > - Merge remote-tracking branch 'centos79/master-cgroup' into master-cgroup > - Disable cgroup.subtree_control testcase on cgroup1 > - Fix gtest > - Merge branch 'master' into master-cgroup > - Merge remote-tracking branch 'f38crac/master-cgroup' into master-cgroup > - ... and 25 more: https://git.openjdk.org/jdk/compare/243cb098...39c90162 > Should I rebase it on top of the commit > [jerboaa@92aaa6f](https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb) > or wait until you finish it? Up to you. I'll keep you posted once the PRs are out. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2085319806
Re: RFR: 8278255: add more warning text in ReentrantLock and ReentrantReadWriteLock [v2]
On Tue, 30 Apr 2024 12:38:02 GMT, Viktor Klang wrote: > @pavelrappo Wdyt? I like it, but maybe native speakers who also write clearly could double check that change from _which_ to _that_? In no particular order: @DougLea, @AlanBateman, @stuart-marks. - PR Comment: https://git.openjdk.org/jdk/pull/18974#issuecomment-2085246810
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Sun, 10 Mar 2024 14:40:09 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 35 commits: > > - Fix whitespace > - Merge branch 'master' into master-cgroup > >Conflicts: > test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp > - Fix gtest > - Update the Java part > - Fix cgroup1 backward compatibility message > - Merge remote-tracking branch 'centos79/master-cgroup' into master-cgroup > - Disable cgroup.subtree_control testcase on cgroup1 > - Fix gtest > - Merge branch 'master' into master-cgroup > - Merge remote-tracking branch 'f38crac/master-cgroup' into master-cgroup > - ... and 25 more: https://git.openjdk.org/jdk/compare/243cb098...39c90162 Should I rebase it on top of the commit https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb or wait until you finish it? - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2085243290
Re: RFR: 8278255: add more warning text in ReentrantLock and ReentrantReadWriteLock [v2]
On Sat, 27 Apr 2024 11:52:18 GMT, Viktor Klang wrote: >> This is an attempt to be more clear about recommendations on Lock usage. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java > > Co-authored-by: Pavel Rappo <32523691+pavelra...@users.noreply.github.com> @pavelrappo Wdyt? - PR Comment: https://git.openjdk.org/jdk/pull/18974#issuecomment-2085221026
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v2]
On Tue, 30 Apr 2024 12:23:36 GMT, Adam Sotona wrote: >> Hi, >> During performance optimization work on Class-File API as JDK lambda >> generator we found some static initialization killers. >> One of them is `java.lang.classfile.Attributes` with tens of static fields >> initialized with individual attribute mappers, and common set of all >> mappers, and static map from attribute names to the mappers. >> >> I propose to turn all the static fields into lazy-initialized static methods >> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` >> static mapping method from the `Attributes` API class. >> >> Please let me know your comments or objections and please review the >> [PR](https://github.com/openjdk/jdk/pull/19006) and >> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into >> 23. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > added bug number Yeah, it has no real usage impact but such changes do require CSRs, like https://bugs.openjdk.org/browse/JDK-8305158 for a `final` on `Arrays` class, so you should include the `final` modifier change in your CSR's specdiff. - PR Comment: https://git.openjdk.org/jdk/pull/19006#issuecomment-2085221255
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v2]
On Tue, 30 Apr 2024 12:13:59 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/BoundAttribute.java
>> line 996:
>>
>>> 994: public static AttributeMapper standardAttribute(Utf8Entry name)
>>> {
>>> 995: // critical bootstrap path, so no lambdas nor method handles
>>> here
>>> 996: return switch (name.hashCode()) {
>>
>> I think we can safely switch over strings, as they are compiled to hashCode
>> switch like what you explicitly have right now. Isn't that the case?
>
> Freshly parsed Utf8Entries conversion to String is expensive and unnecessary.
> We should be very careful when to ask for the conversion as it significantly
> affects some benchmarks.
You are right, I forgot these are Utf8Entry instead of Strings.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1584719802
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Tue, 30 Apr 2024 07:01:16 GMT, Jan Kratochvil wrote: > * Will the patch be accepted only for memory or it has to support also CPU? It should be fine for memory only for a start, but we should allow for on-boarding of other controllers as well. > * Should I code it on top of OpenJDK trunk or on top of > [jerboaa@92aaa6f](https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb) > ? Since this patch solves a similar problem than https://bugs.openjdk.java.net/browse/JDK-8217338 did at the time when only cg v1 was supported I'd feel more comfortable in paying the technical debt and recode it so that cg v1 and cg v2 behaves the same or at least similar. That should make the code cleaner, easier to test and maintain in the long run. So I'm thinking on top of the refactoring. I'll try to prioritize those work items accordingly. Does that sound OK? - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2085186173
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v2]
On Tue, 30 Apr 2024 12:05:46 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added bug number
>
> src/java.base/share/classes/jdk/internal/classfile/impl/BoundAttribute.java
> line 996:
>
>> 994: public static AttributeMapper standardAttribute(Utf8Entry name) {
>> 995: // critical bootstrap path, so no lambdas nor method handles
>> here
>> 996: return switch (name.hashCode()) {
>
> I think we can safely switch over strings, as they are compiled to hashCode
> switch like what you explicitly have right now. Isn't that the case?
Freshly parsed Utf8Entries conversion to String is expensive and unnecessary.
We should be very careful when to ask for the conversion as it significantly
affects some benchmarks.
> test/jdk/jdk/classfile/AttributesTest.java line 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @summary Testing Attributes API.
>
> Can add a line `@bug 8331291`
Added, thank you.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1584696556
PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1584707230
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v2]
> Hi, > During performance optimization work on Class-File API as JDK lambda > generator we found some static initialization killers. > One of them is `java.lang.classfile.Attributes` with tens of static fields > initialized with individual attribute mappers, and common set of all mappers, > and static map from attribute names to the mappers. > > I propose to turn all the static fields into lazy-initialized static methods > and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` > static mapping method from the `Attributes` API class. > > Please let me know your comments or objections and please review the > [PR](https://github.com/openjdk/jdk/pull/19006) and > [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added bug number - Changes: - all: https://git.openjdk.org/jdk/pull/19006/files - new: https://git.openjdk.org/jdk/pull/19006/files/b7b35c5d..27238368 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19006&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19006&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19006.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19006/head:pull/19006 PR: https://git.openjdk.org/jdk/pull/19006
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations
On Tue, 30 Apr 2024 12:09:05 GMT, Chen Liang wrote: > Also the `final` modifier addition should be included in the CSR, as it > changes the access modifiers even if it has no real impact. The class already had only private constructor, so there was no way to extend it. - PR Comment: https://git.openjdk.org/jdk/pull/19006#issuecomment-2085173489
Re: Usage of iconv()
On 2024-04-25 20:30, Philip Race wrote: On 4/24/24 4:24 AM, Magnus Ihse Bursie wrote: That is a good question. libiconv is used only on macOS and AIX, for a few libraries, as you say. I just tried removing -liconv from the macOS dependencies and recompiled, just to see what would happen. There were three instances for macOS: libsplashscreen, libjdwp and libinstrument. libsplashscreen uses it in splashscreen_sys.m, where it is used to convert the jar file name. This is called from the launcher, in java.base/share/native/libjli/java.c libjdwp uses it in utf_util.c, where it is used to convert file name and command lines, iiuc. It is likely that we have similar (but better) ways to convert charsets elsewhere in our libraries that can be used instead of libiconv. I guess these are not the only two places where a filename must be converted from the platform charset to UTF8. So whatever replacement there might be, it needs to be something that is available very early in the life of the VM, in fact before there is a VM running. Agreed. But it seems to be that this is something that needs to be handled by libjli, to properly deal with paths and command lines. I'm wondering if the places which to *not* use iconv (or similar) is actually incorrect. /Magnus -phil.
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations
On Mon, 29 Apr 2024 18:48:53 GMT, Adam Sotona wrote:
> Hi,
> During performance optimization work on Class-File API as JDK lambda
> generator we found some static initialization killers.
> One of them is `java.lang.classfile.Attributes` with tens of static fields
> initialized with individual attribute mappers, and common set of all mappers,
> and static map from attribute names to the mappers.
>
> I propose to turn all the static fields into lazy-initialized static methods
> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)`
> static mapping method from the `Attributes` API class.
>
> Please let me know your comments or objections and please review the
> [PR](https://github.com/openjdk/jdk/pull/19006) and
> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23.
>
> Thank you,
> Adam
Also the `final` modifier addition should be included in the CSR, as it changes
the access modifiers even if it has no real impact.
src/java.base/share/classes/jdk/internal/classfile/impl/BoundAttribute.java
line 996:
> 994: public static AttributeMapper standardAttribute(Utf8Entry name) {
> 995: // critical bootstrap path, so no lambdas nor method handles here
> 996: return switch (name.hashCode()) {
I think we can safely switch over strings, as they are compiled to hashCode
switch like what you explicitly have right now. Isn't that the case?
test/jdk/jdk/classfile/AttributesTest.java line 26:
> 24: /*
> 25: * @test
> 26: * @summary Testing Attributes API.
Can add a line `@bug 8331291`
-
PR Comment: https://git.openjdk.org/jdk/pull/19006#issuecomment-2085160221
PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1584686905
PR Review Comment: https://git.openjdk.org/jdk/pull/19006#discussion_r1584687500
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations
On Mon, 29 Apr 2024 18:48:53 GMT, Adam Sotona wrote: > Hi, > During performance optimization work on Class-File API as JDK lambda > generator we found some static initialization killers. > One of them is `java.lang.classfile.Attributes` with tens of static fields > initialized with individual attribute mappers, and common set of all mappers, > and static map from attribute names to the mappers. > > I propose to turn all the static fields into lazy-initialized static methods > and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` > static mapping method from the `Attributes` API class. > > Please let me know your comments or objections and please review the > [PR](https://github.com/openjdk/jdk/pull/19006) and > [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23. > > Thank you, > Adam Nice changes! Remarks 1. The `INSTANCE` fields should be declared `final`, still safe for lazy initialization. 2. Not from this patch, but the `AttributeStability stability()` overrides can be moved to `AbstractAttributeMapper`, stored in a field, to decrease source code size. 3. `AbstractAttributeMapper` might become sealed now that its implementations are no longer anonymous, might move it to be a private nested class of `Attributes` to omit the long list of permits. - PR Comment: https://git.openjdk.org/jdk/pull/19006#issuecomment-2083832111
RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations
Hi, During performance optimization work on Class-File API as JDK lambda generator we found some static initialization killers. One of them is `java.lang.classfile.Attributes` with tens of static fields initialized with individual attribute mappers, and common set of all mappers, and static map from attribute names to the mappers. I propose to turn all the static fields into lazy-initialized static methods and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` static mapping method from the `Attributes` API class. Please let me know your comments or objections and please review the [PR](https://github.com/openjdk/jdk/pull/19006) and [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into 23. Thank you, Adam - Commit messages: - added impl comment - removed list of predefined attributes - move mappers implementations to AbstractAttributeMapper - 8331291: java.lang.classfile.Attributes class performs a lot of static initializations Changes: https://git.openjdk.org/jdk/pull/19006/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19006&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331291 Stats: 2029 lines in 47 files changed: 904 ins; 619 del; 506 mod Patch: https://git.openjdk.org/jdk/pull/19006.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19006/head:pull/19006 PR: https://git.openjdk.org/jdk/pull/19006
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations
On Mon, 29 Apr 2024 23:04:38 GMT, Chen Liang wrote: > Nice changes! Remarks > > 1. The `INSTANCE` fields should be declared `final`, still safe for lazy > initialization. > 2. Not from this patch, but the `AttributeStability stability()` overrides > can be moved to `AbstractAttributeMapper`, stored in a field, to decrease > source code size. > 3. `AbstractAttributeMapper` might become sealed now that its implementations > are no longer anonymous, might move it to be a private nested class of > `Attributes` to omit the long list of permits. Yes, that is part of the plan for today :) - PR Comment: https://git.openjdk.org/jdk/pull/19006#issuecomment-2084446739
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v5]
> Move all random generators mandated in package `java.util.random` and > currently implemented in module `jdk.random` to module `java.base`, and > remove module `jdk.random`. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Typo. - Changes: - all: https://git.openjdk.org/jdk/pull/18932/files - new: https://git.openjdk.org/jdk/pull/18932/files/d502b245..59c86b43 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18932.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932 PR: https://git.openjdk.org/jdk/pull/18932
RFR: 8308033: The jcmd thread dump related tests should test virtual threads
Can I please get a review of these test-only changes which proposes to remove the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from `ProblemList-Virtual.txt`? When jtreg was enhanced to allow running the tests from within a virtual (main) thread, some tests were problem listed since they either were failing at that time or the test code would require additional work to make it work when the current thread is a virtual thread. The `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which require special handling when the current thread is a virtual thread. `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to use a different command to dump stacktraces when the test runs in a virtual thread. I went back and looked at the original issue for which this test was introduced and based on that, using a different command to dump the stacktraces shouldn't impact what the test was originally intended to test. `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be skipped when the current thread is the virtual thread. This is because the test exercises the `jstack` tool which doesn't print stacktraces of virtual threads and thus the test isn't applicable for virtual threads. I've run these tests with this change, both with platform threads and virtual threads in our CI and the tests complete without failures. - Commit messages: - 8308033: The jcmd thread dump related tests should test virtual threads Changes: https://git.openjdk.org/jdk/pull/19016/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19016&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308033 Stats: 51 lines in 3 files changed: 42 ins; 3 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19016.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19016/head:pull/19016 PR: https://git.openjdk.org/jdk/pull/19016
Re: RFR: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
On Mon, 29 Apr 2024 16:42:05 GMT, Viktor Klang wrote: > This PR finalizes JEP 473 by modifying the PreviewFeature JEP number and > status for Stream Gatherers. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19003#pullrequestreview-2030820148
RFR: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
This PR finalizes JEP 473 by modifying the PreviewFeature JEP number and status for Stream Gatherers. - Commit messages: - Updating PreviewFeature.STREAM_GATHERERS to 473 - Second Preview Changes: https://git.openjdk.org/jdk/pull/19003/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19003&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331346 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19003.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19003/head:pull/19003 PR: https://git.openjdk.org/jdk/pull/19003
Re: RFR: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
On Mon, 29 Apr 2024 16:42:05 GMT, Viktor Klang wrote: > This PR finalizes JEP 473 by modifying the PreviewFeature JEP number and > status for Stream Gatherers. @AlanBateman Let me know if I should create a new JBS issue for this change, or if it is fine to target the JEP JBS issue. 🤔 - PR Comment: https://git.openjdk.org/jdk/pull/19003#issuecomment-2083196525
Re: RFR: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
On Mon, 29 Apr 2024 16:42:05 GMT, Viktor Klang wrote: > This PR finalizes JEP 473 by modifying the PreviewFeature JEP number and > status for Stream Gatherers. LGTM - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19003#pullrequestreview-2030731354
Re: RFR: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
On Mon, 29 Apr 2024 16:42:59 GMT, Viktor Klang wrote: > @AlanBateman Let me know if I should create a new JBS issue for this change, > or if it is fine to target the JEP JBS issue. 🤔 The bot has spotted this already (see "Integration Blocker" in the updated description). You'll need to create a separate JBS issue for the implementation change and link it in JBS to the JEP. - PR Comment: https://git.openjdk.org/jdk/pull/19003#issuecomment-2083249877
Re: RFR: 8316662: Remove one allocation per conversion in Double.toString(double) and Float.toString(float) [v4]
> By correctly sizing an intermediate `byte[]` and making use of the internal > `newStringNoRepl()` method, one allocation per conversion can be avoided when > the runtime uses compact strings. Raffaello Giulietti 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: - Merge branch 'master' into 8316662 - Merge branch 'master' into 8316662 - Uppercase JLA. - 8316662: Remove one allocation per conversion in Double.toString(double) and Float.toString(float) - Changes: - all: https://git.openjdk.org/jdk/pull/15861/files - new: https://git.openjdk.org/jdk/pull/15861/files/87e09f38..4308b78f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15861&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15861&range=02-03 Stats: 1500684 lines in 12654 files changed: 338946 ins; 714685 del; 447053 mod Patch: https://git.openjdk.org/jdk/pull/15861.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15861/head:pull/15861 PR: https://git.openjdk.org/jdk/pull/15861
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131
On Mon, 29 Apr 2024 15:57:02 GMT, Alan Bateman wrote: > This looks like it's adding code to search LD_LIBRARY_PATH on Linux/macOS > too, did you mean to do that? Hi Alan, this first commit of the PR is just a question if Linux/macOS want to participate in this 3rd method. For them it's just a fallback; for AIX it's necessary. If they decide not to participate I just suggest to `#ifdef AIX` the added code. - PR Comment: https://git.openjdk.org/jdk/pull/19000#issuecomment-2084712623
Integrated: 8331264: Reduce java.lang.constant initialization overhead
On Mon, 29 Apr 2024 08:11:06 GMT, Claes Redestad wrote: > I'm looking at ways at reducing/eliminating startup overheads the classfile > API in preparation of #17108, and have pushed a series of enhancements to > that effect already. This PR is a collection of minor improvements which add > up to a 1.5% reduction in retired instructions - or a 5% reduction in > executed bytecode - on a simple lambda startup test. This pull request has now been integrated. Changeset: 0630bb02 Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/0630bb02eb760081ddd612ccb1b12d57d43aab5e Stats: 74 lines in 7 files changed: 7 ins; 5 del; 62 mod 8331264: Reduce java.lang.constant initialization overhead Reviewed-by: liach, mchung - PR: https://git.openjdk.org/jdk/pull/18991
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Sun, 10 Mar 2024 14:40:09 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 35 commits: > > - Fix whitespace > - Merge branch 'master' into master-cgroup > >Conflicts: > test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp > - Fix gtest > - Update the Java part > - Fix cgroup1 backward compatibility message > - Merge remote-tracking branch 'centos79/master-cgroup' into master-cgroup > - Disable cgroup.subtree_control testcase on cgroup1 > - Fix gtest > - Merge branch 'master' into master-cgroup > - Merge remote-tracking branch 'f38crac/master-cgroup' into master-cgroup > - ... and 25 more: https://git.openjdk.org/jdk/compare/243cb098...39c90162 - Will the patch be accepted only for memory or it has to support also CPU? - Should I code it on top of OpenJDK trunk or on top of https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb ? - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2084523889
