Re: Need for a sponsor for JDK-8313674
Hello Íñigo, https://bugs.openjdk.org/browse/JDK-8313674 is already assigned to someone else. Have you checked with them if it's OK to work on this one? -Jaikiran On 25/04/24 9:59 am, Iñigo Mediavilla wrote: Hello, For my first contribution to OpenJDK, I've started looking into JDK-8313674, an issue that falls into core-libs. According to the OpenJDK Developers’ Guide I'd need a sponsor to help me through the contribution process, would anyone be available to help me ? Thanks in advance Íñigo Mediavilla Saiz
Re: Need for a sponsor for JDK-8313674
Thank Alan. I will redirect my email to the other mailing list and include the person who has been assigned to that ticket to see if we can collaborate or he already has a fix. Íñigo El jue, 25 abr 2024, 7:32, Alan Bateman escribió: > On 25/04/2024 05:29, Iñigo Mediavilla wrote: > > Hello, > > For my first contribution to OpenJDK, I've started looking into > JDK-8313674, an issue that falls into core-libs. According to the OpenJDK > Developers’ Guide I'd need a sponsor to help me through the contribution > process, would anyone be available to help me ? > > > That's a request to update a test test to have it check for NVMe devices, > the place to discuss that is nio-dev. I see it has recently been assigned > to someone so it would be polite to check with this person in case they are > already working on it. > > -Alan >
Re: Need for a sponsor for JDK-8313674
On 25/04/2024 05:29, Iñigo Mediavilla wrote: Hello, For my first contribution to OpenJDK, I've started looking into JDK-8313674, an issue that falls into core-libs. According to the OpenJDK Developers’ Guide I'd need a sponsor to help me through the contribution process, would anyone be available to help me ? That's a request to update a test test to have it check for NVMe devices, the place to discuss that is nio-dev. I see it has recently been assigned to someone so it would be polite to check with this person in case they are already working on it. -Alan
Need for a sponsor for JDK-8313674
Hello, For my first contribution to OpenJDK, I've started looking into JDK-8313674, an issue that falls into core-libs. According to the OpenJDK Developers’ Guide I'd need a sponsor to help me through the contribution process, would anyone be available to help me ? Thanks in advance Íñigo Mediavilla Saiz
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 ping, it is already 6+ weeks since last change - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2076286758
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
On Wed, 24 Apr 2024 21:52:11 GMT, Paul Sandoz wrote: > Rather than duplicating some checks I wonder if it is possible to add a > private method `entryByIndex(int index, int expectedTag)` that the existing > `entryByIndex` defers to. If the `expectedTag` is non-negative then it checks > `tag` against `expectedTag` before proceeding to the switch expression. Then > the implementations of `readClassEntry` etc can be adjusted to pass along the > expected tag. Unfortunately it would have to be an expected tags list or an extra constructed bit mask, due to the multiple tags allowed for MemberRefEntry and it would slightly affect the performance. Also an additional info for the exception message would have to be passed down (or the exception would have to be re-thrown), as the tag mask is not very informative. - PR Comment: https://git.openjdk.org/jdk/pull/18907#issuecomment-2076114167
Re: large longs to string
Claes,
> IIUC this optimization leans on 4 long divs being slower than 1 long div + 4
> int divs
Exactly. I think there is also some benefit from unrolling the 4 int
digit pair operations.
> which might not be true on all platforms, nor stay true in the future
Agreed, but I am not sure how to reason about that.
> Long values will in practice likely be biased towards lower values, so it’s
> important that any optimization to .. longer values doesn’t regress inputs in
> the int range. Since it’s all guarded by a test that is already there there
> shouldn’t be much room for a difference, but adding code can cause
> interesting issues so it’s always worth measuring to make sure. Have you run
> any benchmark for inputs smaller than the threshold? And for a healthy mix of
> values?
I had been focused on "longer" values, as I have uses which are
effectively guaranteed to have some bits from 53-63 set.
I added some tests for "int" values as well as a mix with 2 "longs"
and 3 "ints". This showed a pretty small regression for the int and
mixed case. That regression went away by changing back to a while loop
comparing to Integer.MIN_VALUE, and that did not give up much of the
gain.
private static final long[] ints = new long[] {Integer.MAX_VALUE,
12345, -5432, 654321, 5};
private static final long[] longs = new long[] {235789987235L,
235789987235326L, -98975433216803632L, Long.MAX_VALUE};
private static final long[] mixed = new long[] {5, Long.MIN_VALUE,
654321, 9876543210L, 543};
Benchmark (type) Mode Cnt ScoreError Units
LongToStringBenchmark.baseline int avgt3 24.105 ┬▒ 11.751 ns/op
LongToStringBenchmark.baseline long avgt3 51.223 ┬▒ 21.069 ns/op
LongToStringBenchmark.baseline mix avgt3 34.849 ┬▒ 7.270 ns/op
LongToStringBenchmark.change int avgt3 25.846 ┬▒ 2.012 ns/op
LongToStringBenchmark.changelong avgt3 43.053 ┬▒ 13.886 ns/op
LongToStringBenchmark.change mix avgt3 35.826 ┬▒ 2.901 ns/op
LongToStringBenchmark.changeLoop int avgt3 24.261 ┬▒ 7.325 ns/op
LongToStringBenchmark.changeLooplong avgt3 44.254 ┬▒ 22.921 ns/op
LongToStringBenchmark.changeLoop mix avgt3 29.501 ┬▒ 8.693 ns/op
"change" is what I had proposed originally and "changeLoop" is:
while (i <= Integer.MIN_VALUE) {
long q = i / 100_000_000;
charPos -= 8;
write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
v = q;
}
Brett
On Wed, Apr 24, 2024 at 2:39 PM Claes Redestad
wrote:
>
> Hi,
>
> IIUC this optimization leans on 4 long divs being slower than 1 long div + 4
> int divs, which might not be true on all platforms, nor stay true in the
> future. Long values will in practice likely be biased towards lower values,
> so it’s important that any optimization to .. longer values doesn’t regress
> inputs in the int range. Since it’s all guarded by a test that is already
> there there shouldn’t be much room for a difference, but adding code can
> cause interesting issues so it’s always worth measuring to make sure. Have
> you run any benchmark for inputs smaller than the threshold? And for a
> healthy mix of values?
>
> Thanks!
> /Claes
>
> 24 apr. 2024 kl. 21:08 skrev Brett Okken :
>
> Is there interest in optimizing StringLatin1.getChars(long, int, byte[]) for
> large (larger than int) long values[1]?
> We can change this to work with 8 digits at a time, which reduces the amount
> of 64 bit arithmetic required.
>
> if (i <= -1_000_000_000) {
> long q = i / 100_000_000;
> charPos -= 8;
> write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
> i = q;
> if (i <= -1_000_000_000) {
> q = i / 100_000_000;
> charPos -= 8;
> write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
> i = q;
> }
> }
>
>
> A simple implementation of write4DigitPairs would just call the existing
> writeDigitPair method 4 times:
>
>
> private static void write4DigitPairs(byte[] buf, int idx, int value) {
> int v = value;
> int v2 = v / 100;
> writeDigitPair(buf, idx + 6, v - (v2 * 100));
> v = v2;
>
> v2 = v / 100;
> writeDigitPair(buf, idx + 4, v - (v2 * 100));
> v = v2;
>
> v2 = v / 100;
> writeDigitPair(buf, idx + 2, v - (v2 * 100));
> v = v2;
>
> v2 = v / 100;
> writeDigitPair(buf, idx, v - (v2 * 100));
> }
>
> There is the option to OR the 4 short values together into a long and
> leverage a ByteArrayLittleEndian.setLong call, but I see that the previous
> usage of ByteArrayLittleEndian.setShort was removed[2].
>
> A small benchmark of longs which would qualify shows up to 20% improvement.
>
> Presumably a similar change could make sense for StringUTF16, but I have not
> spent any time benchmarking it.
>
> Brett
>
> [1] -
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/StringLatin1.java#L163-L168
> [2] -
> https://github.com/openjdk/jdk/commit/913e43fea995b746fb9e1b25587d254396c7c3c9
>
>
Re: RFR: 8330542: Add two JAXP configuration files in preparation for a secure by default configuration [v5]
> Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > > jaxp-compat.properties: used to regain compatibility from any more > restricted configuration than previous versions such as JDK 22 Joe Wang has updated the pull request incrementally with one additional commit since the last revision: add warning msg that the config files can change or be removed. - Changes: - all: https://git.openjdk.org/jdk/pull/18831/files - new: https://git.openjdk.org/jdk/pull/18831/files/019c2aee..93b66312 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18831=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18831=03-04 Stats: 8 lines in 2 files changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18831.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18831/head:pull/18831 PR: https://git.openjdk.org/jdk/pull/18831
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v25]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: package spec updates, mostly about reference queues and dequeueing - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/cc21d296..16fcc764 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=24 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=23-24 Stats: 15 lines in 1 file changed: 0 ins; 3 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8325373: Improve StackCounter error reporting for bad switch cases
On Tue, 23 Apr 2024 12:59:18 GMT, Adam Sotona wrote:
> ClassFile API `StackMapGenerator` attaches print or hex dump of the method to
> an error message.
> However there is no such attachment when the stack maps generation is turned
> off.
>
> This patch moves class print/dump to `impl.Util::dumpMethod`, so it is shared
> by `StackMapGenerator` and `StackCounter` to provide the same level of
> details in case of an error.
>
> Please review.
>
> Thank you,
> Adam
Looks good, just update the copy write date in Util.java.
src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 229:
> 227: };
> 228: ClassPrinter.toYaml(clm.methods().get(0).code().get(),
> ClassPrinter.Verbosity.TRACE_ALL, dump);
> 229: } catch (Error | Exception suppresed) {
If you like you can replace `suppresed` [sic] with `_`.
-
Marked as reviewed by psandoz (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18914#pullrequestreview-2021078844
PR Review Comment: https://git.openjdk.org/jdk/pull/18914#discussion_r1578604408
Re: RFR: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
On Tue, 23 Apr 2024 11:56:41 GMT, Adam Sotona wrote: > This patch adds missing `@throws` javadoc annotations to > `java.lang.classfile.BufWriter`. > > Please review. > > Thank you, > Adam This looks good, but for completeness it will need a CSR. - PR Review: https://git.openjdk.org/jdk/pull/18913#pullrequestreview-2021068931
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
On Tue, 23 Apr 2024 07:39:47 GMT, Adam Sotona wrote: > ClassFile API dives into the nested constant pool entries without type > restrictions, while parsing a class file. Validation of the entry is > performed post-parsing. Specifically corrupted constant pool entry may cause > infinite loop during parsing and throws SOE. > This patch resolves the issue by providing specific implementations for the > nested CP entries parsing, instead of sharing the common (post-checking) code. > Added test simulates the situation on inner-looped method reference entry. > > Please review. > > Thank you, > Adam Rather than duplicating some checks I wonder if it is possible to add a private method `entryByIndex(int index, int expectedTag)` that the existing `entryByIndex` defers to. If the `expectedTag` is non-negative then it checks `tag` against `expectedTag` before proceeding to the switch expression. Then the implementations of `readClassEntry` etc can be adjusted to pass along the expected tag. - PR Review: https://git.openjdk.org/jdk/pull/18907#pullrequestreview-2021009969
Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v13]
On Wed, 24 Apr 2024 19:53:41 GMT, Vicente Romero wrote: >> Reflection is not retrieving generic type information for mandated >> parameters. This is a known issue which has been explicitly stated in the >> API of some reflection methods. Fix for >> [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the >> parameters of compact constructors of record classes `mandated` as specified >> in the spec. But this implied that users that previous to this change could >> retrieve the generic type of parameters of compact constructors now they >> can't anymore. The proposed fix is to try to retrieve generic type >> information for mandated parameters if available plus changing the spec of >> the related reflection methods. >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/java/lang/reflect/records/RecordReflectionTest.java > > Co-authored-by: Andrey Turbanov Is it possible for us to check the length of `genericParamTypes`, so if `genericParamTypes.length == getParameterCount()`, then we assume the generic parameters are the real generic parameters. This approach would fix the record canonical constructors, and it would benefit other JVM languages that plan to declare generic types on mandated/synthetic parameters for Java's core reflection. - PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2075865476
Re: RFR: 8330686: Fix typos in the DatabaseMetaData javadoc [v3]
On Wed, 24 Apr 2024 08:54:53 GMT, Jin Kwon wrote: >> Fix typos within the `DatabaseMetaData.java`. > > Jin Kwon has updated the pull request incrementally with one additional > commit since the last revision: > > 8330686: Fix typos in the DatabaseMetaData javadoc > > Reviewed-by: jpai Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18860#pullrequestreview-2020930726
Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v10]
On Wed, 24 Apr 2024 15:00:34 GMT, Vicente Romero wrote: >> Vicente Romero has updated the pull request incrementally with one >> additional commit since the last revision: >> >> adding comment to jcod file > > test > Will this fix be backported to JDK 21? @vicente-romero-oracle not sure right now tbh - PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2075730082
Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v13]
> Reflection is not retrieving generic type information for mandated > parameters. This is a known issue which has been explicitly stated in the API > of some reflection methods. Fix for > [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the > parameters of compact constructors of record classes `mandated` as specified > in the spec. But this implied that users that previous to this change could > retrieve the generic type of parameters of compact constructors now they > can't anymore. The proposed fix is to try to retrieve generic type > information for mandated parameters if available plus changing the spec of > the related reflection methods. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/lang/reflect/records/RecordReflectionTest.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/17070/files - new: https://git.openjdk.org/jdk/pull/17070/files/589f3ff5..4c3d4e1e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17070=12 - incr: https://webrevs.openjdk.org/?repo=jdk=17070=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17070/head:pull/17070 PR: https://git.openjdk.org/jdk/pull/17070
Re: large longs to string
Hi,
IIUC this optimization leans on 4 long divs being slower than 1 long div + 4
int divs, which might not be true on all platforms, nor stay true in the
future. Long values will in practice likely be biased towards lower values, so
it’s important that any optimization to .. longer values doesn’t regress inputs
in the int range. Since it’s all guarded by a test that is already there there
shouldn’t be much room for a difference, but adding code can cause interesting
issues so it’s always worth measuring to make sure. Have you run any benchmark
for inputs smaller than the threshold? And for a healthy mix of values?
Thanks!
/Claes
24 apr. 2024 kl. 21:08 skrev Brett Okken :
Is there interest in optimizing StringLatin1.getChars(long, int, byte[]) for
large (larger than int) long values[1]?
We can change this to work with 8 digits at a time, which reduces the amount of
64 bit arithmetic required.
if (i <= -1_000_000_000) {
long q = i / 100_000_000;
charPos -= 8;
write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
i = q;
if (i <= -1_000_000_000) {
q = i / 100_000_000;
charPos -= 8;
write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
i = q;
}
}
A simple implementation of write4DigitPairs would just call the existing
writeDigitPair method 4 times:
private static void write4DigitPairs(byte[] buf, int idx, int value) {
int v = value;
int v2 = v / 100;
writeDigitPair(buf, idx + 6, v - (v2 * 100));
v = v2;
v2 = v / 100;
writeDigitPair(buf, idx + 4, v - (v2 * 100));
v = v2;
v2 = v / 100;
writeDigitPair(buf, idx + 2, v - (v2 * 100));
v = v2;
v2 = v / 100;
writeDigitPair(buf, idx, v - (v2 * 100));
}
There is the option to OR the 4 short values together into a long and leverage
a ByteArrayLittleEndian.setLong call, but I see that the previous usage of
ByteArrayLittleEndian.setShort was removed[2].
A small benchmark of longs which would qualify shows up to 20% improvement.
Presumably a similar change could make sense for StringUTF16, but I have not
spent any time benchmarking it.
Brett
[1] -
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/StringLatin1.java#L163-L168
[2] -
https://github.com/openjdk/jdk/commit/913e43fea995b746fb9e1b25587d254396c7c3c9
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v7]
> Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision: Make deleting file OS agnostic - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/e2ef2a51..ca03ead2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18786=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18786=05-06 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v6]
> Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with two additional commits since the last revision: - Adding test case - Removing long lines - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/3ea56c5c..e2ef2a51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18786=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18786=04-05 Stats: 112 lines in 2 files changed: 106 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v9]
> This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Nits, clean up constant, introduce missing constant MethodTypeDesc for toString - Changes: - all: https://git.openjdk.org/jdk/pull/18690/files - new: https://git.openjdk.org/jdk/pull/18690/files/9742f074..0ac8f24b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18690=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18690=07-08 Stats: 13 lines in 1 file changed: 2 ins; 5 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v8]
On Wed, 24 Apr 2024 10:08:42 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Make Set.of(STRONG) a constant, fix compilation, minor code improvements Thanks for reviewing! I'll split out and PR an ASM version as a subtask and rebase this PR on top of that. - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2075654943
large longs to string
Is there interest in optimizing StringLatin1.getChars(long, int, byte[])
for large (larger than int) long values[1]?
We can change this to work with 8 digits at a time, which reduces the
amount of 64 bit arithmetic required.
if (i <= -1_000_000_000) {
long q = i / 100_000_000;
charPos -= 8;
write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
i = q;
if (i <= -1_000_000_000) {
q = i / 100_000_000;
charPos -= 8;
write4DigitPairs(buf, charPos, (int) ((q * 100_000_000) - i));
i = q;
}
}
A simple implementation of write4DigitPairs would just call the existing
writeDigitPair method 4 times:
private static void write4DigitPairs(byte[] buf, int idx, int value) {
int v = value;
int v2 = v / 100;
writeDigitPair(buf, idx + 6, v - (v2 * 100));
v = v2;
v2 = v / 100;
writeDigitPair(buf, idx + 4, v - (v2 * 100));
v = v2;
v2 = v / 100;
writeDigitPair(buf, idx + 2, v - (v2 * 100));
v = v2;
v2 = v / 100;
writeDigitPair(buf, idx, v - (v2 * 100));
}
There is the option to OR the 4 short values together into a long and
leverage a ByteArrayLittleEndian.setLong call, but I see that the previous
usage of ByteArrayLittleEndian.setShort was removed[2].
A small benchmark of longs which would qualify shows up to 20% improvement.
Presumably a similar change could make sense for StringUTF16, but I have
not spent any time benchmarking it.
Brett
[1] -
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/StringLatin1.java#L163-L168
[2] -
https://github.com/openjdk/jdk/commit/913e43fea995b746fb9e1b25587d254396c7c3c9
Re: RFR: 8330686: Fix typos in the DatabaseMetaData javadoc [v2]
On Tue, 23 Apr 2024 09:32:40 GMT, Jin Kwon wrote: >> Fix typos within the `DatabaseMetaData.java`. > > Jin Kwon has updated the pull request incrementally with one additional > commit since the last revision: > > [JDK-8330686] Update copyright year Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18860#pullrequestreview-2020585117
RFR: 8330276: Console methods with explicit Locale
Proposing new overloaded methods in `java.io.Console` class that explicitly take a `Locale` argument. Existing methods rely on the default locale, so the users won't be able to format their prompts/outputs in a certain locale explicitly. - Commit messages: - Not using System.err - spacing - initial commit Changes: https://git.openjdk.org/jdk/pull/18923/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18923=00 Issue: https://bugs.openjdk.org/browse/JDK-8330276 Stats: 370 lines in 7 files changed: 289 ins; 0 del; 81 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8320575: generic type information lost on mandated parameters [v12]
> Reflection is not retrieving generic type information for mandated > parameters. This is a known issue which has been explicitly stated in the API > of some reflection methods. Fix for > [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the > parameters of compact constructors of record classes `mandated` as specified > in the spec. But this implied that users that previous to this change could > retrieve the generic type of parameters of compact constructors now they > can't anymore. The proposed fix is to try to retrieve generic type > information for mandated parameters if available plus changing the spec of > the related reflection methods. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: making the javadoc less general - Changes: - all: https://git.openjdk.org/jdk/pull/17070/files - new: https://git.openjdk.org/jdk/pull/17070/files/508f0199..589f3ff5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17070=11 - incr: https://webrevs.openjdk.org/?repo=jdk=17070=10-11 Stats: 9 lines in 1 file changed: 3 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17070/head:pull/17070 PR: https://git.openjdk.org/jdk/pull/17070
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v2]
On Tue, 9 Apr 2024 02:01:36 GMT, Anthony Scarpino wrote:
>> Volodymyr Paprotski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> remove use of jdk.crypto.ec
>
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 308:
>
>> 306:
>> 307: /*
>> 308: * public Point addition. Used by ECDSAOperations
>
> Was the old description not applicable anymore? It would be nice to improve
> on the existing description that shortening it.
Forgot to go back and fix the comment. Fixed..
As for the 'meaning'. Notice the signature of the function changed (i.e. no
longer a 'mixed point', but two ProjectivePoints. This is a good idea
regardless of Montgomery, but it affects montgomery particularly badly (need to
compute zInv for 'no reason'. )
For sake of completeness. Apart from constructor, the 'API' for ECOperations
(i.e. as used by ECDHE, ECDSAOperations and KeyGeneration) are these three
functions (everything else is used internally by this class)
public void setSum(MutablePoint p, MutablePoint p2)
public MutablePoint multiply(AffinePoint affineP, byte[] s)
public MutablePoint multiply(ECPoint ecPoint, byte[] s)
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 321:
>
>> 319: ECOperations ops = this;
>> 320: if (this.montgomeryOps != null) {
>> 321: assert p.getField() instanceof
>> IntegerMontgomeryFieldModuloP;
>
> This should throw a ProviderException, I believe this would throw an
> AssertionException
Missed this comment. No longer applicable (this.montgomeryOps got refactored
away)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578144125
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578161140
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v3]
On Tue, 23 Apr 2024 19:55:57 GMT, Anthony Scarpino
wrote:
>> Volodymyr Paprotski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Comments from Jatin and Tony
>
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 204:
>
>> 202: * @return the product
>> 203: */
>> 204: public MutablePoint multiply(AffinePoint affineP, byte[] s) {
>
> It seems like there could be some combining of both `multiply()`. If
> `multiply(AffinePoint, ...)` is called, it can call `DefaultMultiplier` with
> the `affineP`, but internally call the other `multiply(ECPoint, ...)` for the
> other situations. I'd rather not have two methods doing most of the same
> code, but different methods.
Thanks, they indeed look identical, didnt notice. Fixed. (repeated the same
hashmap refactoring and didnt notice I produced identical code twice)
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 467:
>
>> 465: sealed static abstract class SmallWindowMultiplier implements
>> PointMultiplier
>> 466: permits DefaultMultiplier, DefaultMontgomeryMultiplier {
>> 467: private final AffinePoint affineP;
>
> I don't think `affineP` needs to be a class variable anymore. It's only used
> in the constructor
Didn't notice, thanks, fixed.
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 592:
>
>> 590: }
>> 591:
>> 592: private final ProjectivePoint.Immutable[][] points;
>
> Can you define this at the top please.
Done
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 668:
>
>> 666: }
>> 667:
>> 668: private final BigInteger[] base;
>
> Can you define this at the top. You use it in the constructor but it's
> defined later on.
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578117929
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578147190
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578148562
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578150303
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v2]
On Tue, 16 Apr 2024 02:26:57 GMT, Jatin Bhateja wrote: >> Per-above, this is a switch statement (`UNLIKELY`) fallback. I can still add >> alignment and loop rotation, but being a fallback figured its more important >> to keep it small > > It's all part of intrinsic, no harm in polishing it. Done (normalized loop/backedge). There was actually a problem in the loop counter.. (`i-=1` instead of `i-=16`). Can't include a test since classes are sealed, but verified manually. - PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578172873
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v4]
> Performance. Before: > > Benchmark(algorithm) (dataSize) (keyLength) > (provider) Mode Cnt ScoreError Units > SignatureBench.ECDSA.signSHA256withECDSA1024 256 > thrpt3 6443.934 ± 6.491 ops/s > SignatureBench.ECDSA.signSHA256withECDSA 16384 256 > thrpt3 6152.979 ± 4.954 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA1024 256 > thrpt3 1895.410 ± 36.979 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 > thrpt3 1878.955 ± 45.487 ops/s > Benchmark(algorithm) (keyLength) > (kpgAlgorithm) (provider) Mode Cnt ScoreError Units > o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1357.810 ± 26.584 ops/s > o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1352.119 ± 23.547 ops/s > Benchmark (isMontBench) Mode Cnt Score > Error Units > PolynomialP256Bench.benchMultiply false thrpt3 1746.126 ± > 10.970 ops/s > > Performance, no intrinsic: > > Benchmark(algorithm) (dataSize) (keyLength) > (provider) Mode Cnt Score Error Units > SignatureBench.ECDSA.signSHA256withECDSA1024 256 > thrpt3 6529.839 ± 42.420 ops/s > SignatureBench.ECDSA.signSHA256withECDSA 16384 256 > thrpt3 6199.747 ± 133.566 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA1024 256 > thrpt3 1973.676 ± 54.071 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 > thrpt3 1932.127 ± 35.920 ops/s > Benchmark(algorithm) (keyLength) > (kpgAlgorithm) (provider) Mode Cnt ScoreError Units > o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1355.788 ± 29.858 ops/s > o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1346.523 ± 28.722 ops/s > Benchmark (isMontBench) Mode Cnt Score > Error Units > PolynomialP256Bench.benchMultiply true thrpt3 1919.574 ± > 10.591 ops/s > > Performance, **with intrinsics*... Volodymyr Paprotski has updated the pull request incrementally with one additional commit since the last revision: Comments from Tony and Jatin - Changes: - all: https://git.openjdk.org/jdk/pull/18583/files - new: https://git.openjdk.org/jdk/pull/18583/files/6f9ac046..c93a71f0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18583=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18583=02-03 Stats: 48 lines in 2 files changed: 20 ins; 20 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18583/head:pull/18583 PR: https://git.openjdk.org/jdk/pull/18583
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v5]
On Wed, 24 Apr 2024 14:49:55 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Using new macro to avoid reporting JNI error Looks good. Can you add a new test for this? You can reference MainClassCantBeLoadedTest.java which does something similar to what you need. src/java.base/share/native/libjli/java.c line 621: > 619: helperClass = GetLauncherHelperClass(env); > 620: isStaticMainField = > 621: (*env)->GetStaticFieldID(env, helperClass, "isStaticMain", "Z"); Nit: this line isn't long. Can do in 1 line. Same for line 623-624, 626-627. - PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2020321074 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1578158252
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v4]
On Wed, 24 Apr 2024 15:45:58 GMT, Brian Burkhalter wrote: >> Prevent blocking due to a carrier thread not being released when >> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. > > Brian Burkhalter has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8330748: Modify writeTo() not to invoke toByteArray() > - Merge > - 8330748: Add vthread1.join() in test > - Correct ID in test @bug tag > - 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier Running tests now. Assuming those pan out, will likely integrate tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/18901#issuecomment-2075392686
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v8]
On Wed, 24 Apr 2024 10:08:42 GMT, Claes Redestad wrote:
>> This patch suggests a workaround to an issue with huge SCF MH expression
>> trees taking excessive JIT compilation resources by reviving (part of) the
>> simple bytecode-generating strategy that was originally available as an
>> all-or-nothing strategy choice.
>>
>> Instead of reintroducing a binary strategy choice I propose a threshold
>> parameter, controlled by
>> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions
>> below or at this threshold there's no change, for expressions with an arity
>> above it we use the `StringBuilder`-chain bytecode generator.
>>
>> There are a few trade-offs at play here which influence the choice of
>> threshold. The simple high arity strategy will for example not see any reuse
>> of LambdaForms but strictly always generate a class per indy callsite, which
>> means we might end up with a higher total number of classes generated and
>> loaded in applications if we set this value too low. It may also produce
>> worse performance on average. On the other hand there is the observed
>> increase in C2 resource usage as expressions grow unwieldy. On the other
>> other hand high arity expressions are likely rare to begin with, with less
>> opportunities for sharing than the more common low-arity expressions.
>>
>> I turned the submitted test case into a few JMH benchmarks and did some
>> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`:
>>
>> Baseline strategy:
>> 13 args: 6.3M
>> 23 args: 18M
>> 123 args: 868M
>>
>> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`:
>> 13 args: 2.11M
>> 23 args: 3.67M
>> 123 args: 4.75M
>>
>> For 123 args the memory overhead of the baseline strategy is 180x, but for
>> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13
>> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb)
>> I've conservatively chosen a threshold at arity 20. This keeps C2 resource
>> pressure at a reasonable level (< 18M) while avoiding perturbing performance
>> at the vast majority of call sites.
>>
>> I was asked to use the new class file API for mainline. There's a version of
>> this patch implemented using ASM in 7c52a9f which might be a reasonable
>> basis for a backport.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make Set.of(STRONG) a constant, fix compilation, minor code improvements
Looks fine to me. Indeed, splitting this with ASM and then convert it to
ClassFile API would help the backport.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1077:
> 1075:
> 1076: /**
> 1077: * Ensure a capacity in the initial StringBuilder to
> accommodate all constants plus this factor times the number
Nit: wrap long line.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1085:
> 1083:
> 1084: static {
> 1085: DUMPER =
> ClassFileDumper.getInstance("java.lang.invoke.StringConcatFactory.dump",
> "stringConcatClasses");
Nit: this static block isn't strictly needed. Can assign at the declaration of
the static variable.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1112:
> 1110: return hiddenLookup.findStatic(innerClass, METHOD_NAME,
> args);
> : } catch (Exception e) {
> 1112: DUMPER.dumpFailedClass(className, classBytes);
This line is no longer needed. The bytes will be dumped if it's enabled for
both success and failing case.
-
Marked as reviewed by mchung (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18690#pullrequestreview-2020345792
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1578178759
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1578176295
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1578173742
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v9]
On Wed, 24 Apr 2024 16:11:35 GMT, Jaikiran Pai wrote: > With inputs from Lance, I've updated the text and the summary of the release > note as per the guidelines. You can now mark it as "Resolved", "Delivered". Done - thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-2075354891
Re: RFR: 8320575: generic type information lost on mandated parameters [v11]
> Reflection is not retrieving generic type information for mandated > parameters. This is a known issue which has been explicitly stated in the API > of some reflection methods. Fix for > [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the > parameters of compact constructors of record classes `mandated` as specified > in the spec. But this implied that users that previous to this change could > retrieve the generic type of parameters of compact constructors now they > can't anymore. The proposed fix is to try to retrieve generic type > information for mandated parameters if available plus changing the spec of > the related reflection methods. > > TIA Vicente Romero 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 11 additional commits since the last revision: - Merge branch 'master' into JDK-8320575 - adding comment to jcod file - adding a comment to the test - addressing review comments - fixing comment - simplifying code - removing comment - javadoc - javadoc - javadoc adjustments - ... and 1 more: https://git.openjdk.org/jdk/compare/23c8a258...508f0199 - Changes: - all: https://git.openjdk.org/jdk/pull/17070/files - new: https://git.openjdk.org/jdk/pull/17070/files/f6e837d3..508f0199 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17070=10 - incr: https://webrevs.openjdk.org/?repo=jdk=17070=09-10 Stats: 682824 lines in 8459 files changed: 153181 ins; 165583 del; 364060 mod Patch: https://git.openjdk.org/jdk/pull/17070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17070/head:pull/17070 PR: https://git.openjdk.org/jdk/pull/17070
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v9]
On Tue, 23 Apr 2024 12:49:15 GMT, Jaikiran Pai wrote: >> Archie Cobbs 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 10 additional >> commits since the last revision: >> >> - Merge branch 'master' into JDK-7036144 >> - Back-out Javadoc addition; to be added in a separate issue. >> - Document the handling of concatenated streams. >> - Merge branch 'master' into JDK-7036144 >> - Merge branch 'master' into JDK-7036144 >> - Merge branch 'master' into JDK-7036144 >> - Address third round of review comments. >> - Address second round of review comments. >> - Address review comments. >> - Fix bug in GZIPInputStream when underlying available() returns short. > > Hello Archie, we forgot to create a release note for this one (there's still > time). Would you be willing to create one, following the instructions here > https://openjdk.org/guide/#release-notes? If you need any help, let us know. > One of us will review that release note before you can Resolve it to > Delivered. > Hi @jaikiran, > > No problem - please see > [JDK-8330995](https://bugs.openjdk.org/browse/JDK-8330995) and let me know if > anything else is needed. Thank you Archie. With inputs from Lance, I've updated the text and the summary of the release note as per the guidelines. You can now mark it as "Resolved", "Delivered". - PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-2075318205
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v4]
On Wed, 24 Apr 2024 15:45:58 GMT, Brian Burkhalter wrote: >> Prevent blocking due to a carrier thread not being released when >> `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. > > Brian Burkhalter has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8330748: Modify writeTo() not to invoke toByteArray() > - Merge > - 8330748: Add vthread1.join() in test > - Correct ID in test @bug tag > - 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier Updated version looks fine. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18901#pullrequestreview-2020325265
Re: RFR: 8330542: Add two JAXP configuration files in preparation for a secure by default configuration [v4]
On Wed, 24 Apr 2024 14:07:43 GMT, Sean Mullan wrote: > > Sounds good, can you add an example to the RN using the above system property? Added. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2075305341
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v4]
> Prevent blocking due to a carrier thread not being released when > `ByteArrayOutputStream.writeTo` is invoked from a virtual thread. Brian Burkhalter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - 8330748: Modify writeTo() not to invoke toByteArray() - Merge - 8330748: Add vthread1.join() in test - Correct ID in test @bug tag - 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier - Changes: - all: https://git.openjdk.org/jdk/pull/18901/files - new: https://git.openjdk.org/jdk/pull/18901/files/1dd59b7b..8076291f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18901=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18901=02-03 Stats: 8598 lines in 278 files changed: 4899 ins; 2693 del; 1006 mod Patch: https://git.openjdk.org/jdk/pull/18901.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18901/head:pull/18901 PR: https://git.openjdk.org/jdk/pull/18901
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Wed, 24 Apr 2024 14:54:34 GMT, Viktor Klang wrote:
>> Currently we have
>>
>> public void writeTo(OutputStream out) throws IOException {
>> if (Thread.currentThread().isVirtual()) {
>> out.write(toByteArray());
>> } else synchronized (this) {
>> out.write(buf, 0, count);
>> }
>> }
>>
>> where `toByteArray()` is `synchronized`, but here I would think that we'd
>> want to replace it with simply `Arrays.copyOf(buf, count)` without the
>> `synchronized`, no?
>
> @bplb My interpretation was that we didn't want to hold the monitor *during*
> out.write().
Please see 8076291fb3d097ef67d0b59b9be3c8b762aad7cf.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1578116805
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
On Wed, 24 Apr 2024 13:50:56 GMT, Raffaello Giulietti wrote: > 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`. In terms of .jmod, the footprint of java.base.jmod is 20'542'561 bytes in the OpenJDK 22.0.1 build, and 20'552'354 bytes on my local 23 build, a difference of about 10 KB, or around 0.05%. Just renamed the package to `jdk.internal.random`. - PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2075209956
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v2]
> 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: Renamed package jdk.random to jdk.internal.random. - Changes: - all: https://git.openjdk.org/jdk/pull/18932/files - new: https://git.openjdk.org/jdk/pull/18932/files/7f45c525..ba6d052c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18932=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18932=00-01 Stats: 30 lines in 11 files changed: 0 ins; 0 del; 30 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
Re: RFR: 8320575: generic type information lost on mandated parameters [v10]
On Thu, 14 Dec 2023 04:00:58 GMT, Vicente Romero wrote: >> Reflection is not retrieving generic type information for mandated >> parameters. This is a known issue which has been explicitly stated in the >> API of some reflection methods. Fix for >> [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the >> parameters of compact constructors of record classes `mandated` as specified >> in the spec. But this implied that users that previous to this change could >> retrieve the generic type of parameters of compact constructors now they >> can't anymore. The proposed fix is to try to retrieve generic type >> information for mandated parameters if available plus changing the spec of >> the related reflection methods. >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding comment to jcod file test - PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2075158188
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Wed, 24 Apr 2024 14:52:45 GMT, Brian Burkhalter wrote:
>> Using a subclass to count the number of invocations of toByteArray seems a
>> bit strange but in general it is more robust to not rely on a method that
>> may be overridden by a subclass. So I think the suggestion is good.
>
> Currently we have
>
> public void writeTo(OutputStream out) throws IOException {
> if (Thread.currentThread().isVirtual()) {
> out.write(toByteArray());
> } else synchronized (this) {
> out.write(buf, 0, count);
> }
> }
>
> where `toByteArray()` is `synchronized`, but here I would think that we'd
> want to replace it with simply `Arrays.copyOf(buf, count)` without the
> `synchronized`, no?
@bplb My interpretation was that we didn't want to hold the monitor *during*
out.write().
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1578034781
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Wed, 24 Apr 2024 07:08:20 GMT, Alan Bateman wrote:
>> So do we think it better not to invoke `toByteArray` here?
>
> Using a subclass to count the number of invocations of toByteArray seems a
> bit strange but in general it is more robust to not rely on a method that may
> be overridden by a subclass. So I think the suggestion is good.
Currently we have
public void writeTo(OutputStream out) throws IOException {
if (Thread.currentThread().isVirtual()) {
out.write(toByteArray());
} else synchronized (this) {
out.write(buf, 0, count);
}
}
where `toByteArray()` is `synchronized`, but here I would think that we'd want
to replace it with simply `Arrays.copyOf(buf, count)` without the
`synchronized`, no?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1578031656
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v5]
> Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision: Using new macro to avoid reporting JNI error - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/66942238..3ea56c5c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18786=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18786=03-04 Stats: 19 lines in 1 file changed: 9 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
On Wed, 24 Apr 2024 13:50:56 GMT, Raffaello Giulietti wrote: > 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`. What is the footprint implications for this? I'm trying to recall what the reasons were for doing this split in the first place. If the implementations are moving to java.base then I suppose they go into jdk.internal.random rather than jdk.random. - PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2075112923
Re: RFR: 8330542: Add two JAXP configuration files in preparation for a secure by default configuration [v4]
On Tue, 23 Apr 2024 19:26:05 GMT, Lance Andersen wrote: > > Also, how does one try out these property files? Is there a system property > > that needs to be set? Can you add more details to the RN on that? > > java -Djava.xml.config.file=$JAVA_HOME/conf/jaxp-compat.properties > > The property was added in JDK 22 see: > https://docs.oracle.com/en/java/javase/22/docs/api/java.xml/module-summary.html#Conf_CF_SP Sounds good, can you add an example to the RN using the above system property? - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2075040607
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v5]
On Tue, 23 Apr 2024 14:31:44 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust output I changed the added output to 'JRE path' to have more consistency with the existing JLI trace messages. - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2075012914
RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
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`. - Commit messages: - 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module Changes: https://git.openjdk.org/jdk/pull/18932/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18932=00 Issue: https://bugs.openjdk.org/browse/JDK-8330005 Stats: 78 lines in 12 files changed: 11 ins; 65 del; 2 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
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v13]
On Wed, 24 Apr 2024 12:00:47 GMT, Evemose wrote:
>> **Subject**
>> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
>> `java.util.List`
>>
>> **Motivation**
>> The motivation behind this proposal is to enhance the functionality of the
>> `List` interface by providing a more flexible way to find the index of an
>> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
>> object as a parameter. This limits the flexibility of these methods as they
>> can only find the index of exact object matches.
>>
>> The proposed methods would accept a `Predicate` as a parameter, allowing
>> users to define a condition that the desired element must meet. This would
>> provide a more flexible and powerful way to find the index of an element in
>> a list.
>>
>> Here is a brief overview of the changes made in this pull request:
>>
>> 1. Added the `indexOf(Predicate filter)` method to the `List`
>> interface.
>> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
>> interface.
>> 3. Implemented these methods in all non-abstract classes that implement the
>> `List` interface.
>>
>> The changes have been thoroughly tested to ensure they work as expected and
>> do not introduce any regressions. The test cases cover a variety of
>> scenarios to ensure the robustness of the implementation.
>>
>> For example, consider the following test case:
>>
>> List list = new ArrayList<>();
>> list.add("Object one");
>> list.add("NotObject two");
>> list.add("NotObject three");
>>
>> int index1 = list.indexOf(s -> s.contains("ct t"));
>> System.out.println(index1); // Expected output: 1
>> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
>> System.out.println(index2); // Expected output: 2
>>
>>
>> Currently, to achieve the same result, we would have to use a more verbose
>> approach:
>>
>> int index1 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).contains("ct t"))
>> .findFirst()
>> .orElse(-1);
>> System.out.println(index1); // Output: 1
>> int index2 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).startsWith("NotObject"))
>> .reduce((first, second) -> second)
>> .orElse(-1);
>> System.out.println(index2); // Output: 2
>>
>>
>> I believe these additions would greatly enhance the functionality and
>> flexibility of the `List` interface, making it more powerful and
>> user-friendly. I look forward to your feedback and am open to making any
>> necessary changes bas...
>
> Evemose has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - ArrayList made findIndexInRange private
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - ArrayList made findLastIndexInRange private
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
Seems like run failed due to internal problems of openjdk. I will trigger rerun
a bit later.
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2074989809
Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf Thank you Matthias, for this change. Lance has run some internal CI tests and they have come back fine. I am in the process of running some more CI tests with this change and I should have the results, very likely by tomorrow. Please wait for those results before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/18908#issuecomment-2074986166
Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests [v2]
On Wed, 24 Apr 2024 13:14:02 GMT, Ludvig Janiuk wrote: > While not a blocker IMO, I'm curious about the issues for the removed lines. > Taking the first one as an example, I see it's "unresolved" (JDK-8192647) but > the file was removed in JDK-8289764. I don't see any other mentions of > "problemlist" in JDK-8192647 so the "problemlist" label should probably also > be removed. > > I think it would be good to just do a check through the other issues and see > if any other bookkeeping needs to be done, or if any surprises pop up. Ok, I'm pinging people here who git blame associates with some of the removed entries: @walulyai , @lmesnik @kumarabhi006 However, I don't see how removing these entries can cause any problems. Someone who have noticed failing problem listed tests by now. - PR Comment: https://git.openjdk.org/jdk/pull/18879#issuecomment-2074947452
Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests [v2]
On Wed, 24 Apr 2024 10:50:44 GMT, Doug Simon wrote: >> This PR adds a check for the format of ProblemList files and ensures they >> only have entries referring to existing tests. >> >> The cleanups in the second commit of this PR were done based on the output >> of `CheckProblemLists`: >> >>> make test TEST=build/problemLists/CheckProblemLists.java >> ... >> STDOUT: >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt >> Checking >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt >> Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt >> Checked 13 problem list files >> Test roots: >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk >> /Users/dnsimon/dev/jdk-jdk/open/test/lib-test >> /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test >> /Users/dnsimon/dev/jdk-jdk/open/test/jaxp >> /Users/dnsimon/dev/jdk-jdk/open/test/langtools >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg >> Following errors found: >> /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: >> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under >> any test root >> vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all >> >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: >> TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, >> issueId=000] duplicates >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76 >> java/util/Properties/StoreReproducibilityTest.java 000 generic-all >> >> /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: >> java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any >> test root >> java/lang/management/MemoryMXBean/PendingAllGC.sh 8158837 >> generic-all >> >> ... > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > removed CheckProblemLists.java While not a blocker IMO, I'm curious about the issues for the removed lines. Taking the first one as an example, I see it's "unresolved" (JDK-8192647) but the file was removed in JDK-8289764. I don't see any other mentions of "problemlist" in JDK-8192647 so the "problemlist" label should probably also be removed. I think it would be good to just do a check through the other issues and see if any other bookkeeping needs to be done, or if any surprises pop up. - PR Comment: https://git.openjdk.org/jdk/pull/18879#issuecomment-2074921452
Integrated: 8314592: Add shortcut to SymbolLookup::find
On Mon, 25 Mar 2024 14:56:23 GMT, Per Minborg wrote: > While `SymbolLookup` correctly uses an `Optional` return to denote whether a > symbol has been found by the lookup or not (which enables composition of > symbol lookups), many clients end up just calling `Optional::get`, or > `Optional::orElseThrow()` on the result. > > This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that > will do a lookup and, if no symbol can be found, throws an > `IllegalArgumentException` with a relevant exception message. This pull request has now been integrated. Changeset: e923dfe4 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/e923dfe4c51291099d9b7411e6c9f20be79b9a53 Stats: 151 lines in 22 files changed: 88 ins; 0 del; 63 mod 8314592: Add shortcut to SymbolLookup::find Reviewed-by: jvernee, prr - PR: https://git.openjdk.org/jdk/pull/18474
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v7]
On Mon, 22 Apr 2024 13:46:59 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Simplify tests
>> - Add a test for null arg
>> - Add a test for findOrThrow()
>> - Merge branch 'master' into symbol-lookup-findorthrow
>> - Change exception type
>> - Update src/java.base/share/classes/java/lang/foreign/package-info.java
>>
>>Co-authored-by: Jorn Vernee
>> - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java
>>
>>Co-authored-by: Maurizio Cimadamore
>> <54672762+mcimadam...@users.noreply.github.com>
>> - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java
>>
>>Co-authored-by: Maurizio Cimadamore
>> <54672762+mcimadam...@users.noreply.github.com>
>> - Fix typo
>> - Update after PR comments
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/099a64e8...0e06e0d6
>
> test/jdk/java/foreign/loaderLookup/TestSymbolLookupFindOrThrow.java line 41:
>
>> 39:
>> 40: static {
>> 41: System.loadLibrary("Foo");
>
> Where is this lib defined?
it is defined in the sub-folder `lookup` in the file `libFoo.c`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1577755855
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v13]
> **Subject**
> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
> `java.util.List`
>
> **Motivation**
> The motivation behind this proposal is to enhance the functionality of the
> `List` interface by providing a more flexible way to find the index of an
> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
> object as a parameter. This limits the flexibility of these methods as they
> can only find the index of exact object matches.
>
> The proposed methods would accept a `Predicate` as a parameter, allowing
> users to define a condition that the desired element must meet. This would
> provide a more flexible and powerful way to find the index of an element in a
> list.
>
> Here is a brief overview of the changes made in this pull request:
>
> 1. Added the `indexOf(Predicate filter)` method to the `List`
> interface.
> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
> interface.
> 3. Implemented these methods in all non-abstract classes that implement the
> `List` interface.
>
> The changes have been thoroughly tested to ensure they work as expected and
> do not introduce any regressions. The test cases cover a variety of scenarios
> to ensure the robustness of the implementation.
>
> For example, consider the following test case:
>
> List list = new ArrayList<>();
> list.add("Object one");
> list.add("NotObject two");
> list.add("NotObject three");
>
> int index1 = list.indexOf(s -> s.contains("ct t"));
> System.out.println(index1); // Expected output: 1
> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
> System.out.println(index2); // Expected output: 2
>
>
> Currently, to achieve the same result, we would have to use a more verbose
> approach:
>
> int index1 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).contains("ct t"))
> .findFirst()
> .orElse(-1);
> System.out.println(index1); // Output: 1
> int index2 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).startsWith("NotObject"))
> .reduce((first, second) -> second)
> .orElse(-1);
> System.out.println(index2); // Output: 2
>
>
> I believe these additions would greatly enhance the functionality and
> flexibility of the `List` interface, making it more powerful and
> user-friendly. I look forward to your feedback and am open to making any
> necessary changes based on your suggestions.
>
> Thank you for considering this proposal.
>
> Best regards
>
> PS: In ...
Evemose has updated the pull request incrementally with two additional commits
since the last revision:
- ArrayList made findIndexInRange private
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- ArrayList made findLastIndexInRange private
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18639/files
- new: https://git.openjdk.org/jdk/pull/18639/files/349ee6bd..d2f358b3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18639=12
- incr: https://webrevs.openjdk.org/?repo=jdk=18639=11-12
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/18639.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18639/head:pull/18639
PR: https://git.openjdk.org/jdk/pull/18639
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v12]
On Tue, 23 Apr 2024 13:54:42 GMT, Evemose wrote:
>> **Subject**
>> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
>> `java.util.List`
>>
>> **Motivation**
>> The motivation behind this proposal is to enhance the functionality of the
>> `List` interface by providing a more flexible way to find the index of an
>> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
>> object as a parameter. This limits the flexibility of these methods as they
>> can only find the index of exact object matches.
>>
>> The proposed methods would accept a `Predicate` as a parameter, allowing
>> users to define a condition that the desired element must meet. This would
>> provide a more flexible and powerful way to find the index of an element in
>> a list.
>>
>> Here is a brief overview of the changes made in this pull request:
>>
>> 1. Added the `indexOf(Predicate filter)` method to the `List`
>> interface.
>> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
>> interface.
>> 3. Implemented these methods in all non-abstract classes that implement the
>> `List` interface.
>>
>> The changes have been thoroughly tested to ensure they work as expected and
>> do not introduce any regressions. The test cases cover a variety of
>> scenarios to ensure the robustness of the implementation.
>>
>> For example, consider the following test case:
>>
>> List list = new ArrayList<>();
>> list.add("Object one");
>> list.add("NotObject two");
>> list.add("NotObject three");
>>
>> int index1 = list.indexOf(s -> s.contains("ct t"));
>> System.out.println(index1); // Expected output: 1
>> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
>> System.out.println(index2); // Expected output: 2
>>
>>
>> Currently, to achieve the same result, we would have to use a more verbose
>> approach:
>>
>> int index1 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).contains("ct t"))
>> .findFirst()
>> .orElse(-1);
>> System.out.println(index1); // Output: 1
>> int index2 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).startsWith("NotObject"))
>> .reduce((first, second) -> second)
>> .orElse(-1);
>> System.out.println(index2); // Output: 2
>>
>>
>> I believe these additions would greatly enhance the functionality and
>> flexibility of the `List` interface, making it more powerful and
>> user-friendly. I look forward to your feedback and am open to making any
>> necessary changes bas...
>
> Evemose has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Added Objects import to sun List
> - Replaced on-demand import in com.sunList
> - added non-null assertions
Commited proposed changes. Also still would appreciate any help about what to
write it csr template (you can see what I figured a few messages above)
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2074774403
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v8]
> While `SymbolLookup` correctly uses an `Optional` return to denote whether a > symbol has been found by the lookup or not (which enables composition of > symbol lookups), many clients end up just calling `Optional::get`, or > `Optional::orElseThrow()` on the result. > > This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that > will do a lookup and, if no symbol can be found, throws an > `IllegalArgumentException` with a relevant exception message. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove redundant test - Changes: - all: https://git.openjdk.org/jdk/pull/18474/files - new: https://git.openjdk.org/jdk/pull/18474/files/0e06e0d6..31d92589 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18474=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18474=06-07 Stats: 7 lines in 1 file changed: 0 ins; 7 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18474.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18474/head:pull/18474 PR: https://git.openjdk.org/jdk/pull/18474
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v12]
On Wed, 24 Apr 2024 08:29:56 GMT, Evemose wrote:
>> src/java.base/share/classes/java/util/ArrayList.java line 380:
>>
>>> 378: }
>>> 379:
>>> 380: int findLastIndexInRange(Predicate filter, int start,
>>> int end) {
>>
>> Suggestion:
>>
>> private int findLastIndexInRange(Predicate filter, int start,
>> int end) {
>
> Yeah i thought about it but indexOfRange isnt private here so either there is
> a point in it or its just legacy without any particular meaning
It is legacy to avoid bridge generation from the SubList.
Before introduction of nestmates in JDK 11, private methods and fields called
by inner classes should be declared package-private, as javac has to generate
bridge methods at each call site to abide to JVM rules (inner classes are just
another class in the package so couldn't call private methods).
This is also the reason you can see patterns like
private static class Holder {
static final Value instance = Value.initialize();
}
where the `static final` is not `private` qualified.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18639#discussion_r1577751899
Re: Usage of iconv()
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. Out of these, libinstrument compiled and linked fine without the -liconv argument. It looks like iconv is referenced in unix/.../EncodingSupport_md.c, but otoh it looks like it is as much (or as little) referenced on macOS as on linux (where we never have linked with -liconv) so it is perhaps just dead code. I did not study it in detail. The code looks very much duplicated from libjdwp. The other two actually failed linking, so they do use libiconv. libsplashscreen uses it in splashscreen_sys.m, where it is used to convert the jar file name. 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. /Magnus On 2024-04-23 14:11, Bernd Eckenfels wrote: Du to a glibc security alert about a charset in iconv() I checked OpenJDK (since I was quite sure encoding is handled in JCL), however there are a few utilities (related to libinstrument and splash Screens) which use iconv. If I see it correctly it’s mostly used for utf8 so it should not expose this particular globe weakness, but I still wonder if that dependency is needed. For some platforms like AIX it even drags on an additional library dependency. (Not to mention different charger tables and especially ugly locale dependencies for containers). Gruß Bernd — https://bernd.eckenfels.net
Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf LGTM. - Marked as reviewed by lucy (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18908#pullrequestreview-2019585061
Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests
On Sun, 21 Apr 2024 22:00:52 GMT, Doug Simon wrote: > This PR adds a check for the format of ProblemList files and ensures they > only have entries referring to existing tests. > > The cleanups in the second commit of this PR were done based on the output of > `CheckProblemLists`: > >> make test TEST=build/problemLists/CheckProblemLists.java > ... > STDOUT: > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt > Checked 13 problem list files > Test roots: > /Users/dnsimon/dev/jdk-jdk/open/test/jdk > /Users/dnsimon/dev/jdk-jdk/open/test/lib-test > /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test > /Users/dnsimon/dev/jdk-jdk/open/test/jaxp > /Users/dnsimon/dev/jdk-jdk/open/test/langtools > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg > Following errors found: > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: > vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under > any test root > vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all > > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: > TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, > issueId=000] duplicates > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76 > java/util/Properties/StoreReproducibilityTest.java 000 generic-all > > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: > java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any > test root > java/lang/management/MemoryMXBean/PendingAllGC.sh 8158837 > generic-all > > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:667: > javax/swing/JFi... I've removed `CheckProblemLists.java` as it overlaps with https://bugs.openjdk.org/browse/CODETOOLS-7903659. - PR Comment: https://git.openjdk.org/jdk/pull/18879#issuecomment-2074660269
Re: RFR: 8330755: ProblemList files have entries referring to non-existent tests [v2]
> This PR adds a check for the format of ProblemList files and ensures they > only have entries referring to existing tests. > > The cleanups in the second commit of this PR were done based on the output of > `CheckProblemLists`: > >> make test TEST=build/problemLists/CheckProblemLists.java > ... > STDOUT: > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Virtual.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-Xcomp.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-generational-zgc.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList-zgc.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jaxp/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Xcomp.txt > Checking > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-generational-zgc.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-zgc.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/langtools/ProblemList.txt > Checking /Users/dnsimon/dev/jdk-jdk/open/test/lib-test/ProblemList.txt > Checked 13 problem list files > Test roots: > /Users/dnsimon/dev/jdk-jdk/open/test/jdk > /Users/dnsimon/dev/jdk-jdk/open/test/lib-test > /Users/dnsimon/dev/jdk-jdk/open/test/failure_handler/test > /Users/dnsimon/dev/jdk-jdk/open/test/jaxp > /Users/dnsimon/dev/jdk-jdk/open/test/langtools > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg > Following errors found: > /Users/dnsimon/dev/jdk-jdk/open/test/hotspot/jtreg/ProblemList.txt:174: > vmTestbase/gc/lock/jni/jnilock002/TestDescription.java does not exist under > any test root > vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 8192647 generic-all > > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:77: > TestAndIssue[test=java/util/Properties/StoreReproducibilityTest.java, > issueId=000] duplicates > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList-Virtual.txt:76 > java/util/Properties/StoreReproducibilityTest.java 000 generic-all > > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:516: > java/lang/management/MemoryMXBean/PendingAllGC.sh does not exist under any > test root > java/lang/management/MemoryMXBean/PendingAllGC.sh 8158837 > generic-all > > /Users/dnsimon/dev/jdk-jdk/open/test/jdk/ProblemList.txt:667: > javax/swing/JFi... Doug Simon has updated the pull request incrementally with one additional commit since the last revision: removed CheckProblemLists.java - Changes: - all: https://git.openjdk.org/jdk/pull/18879/files - new: https://git.openjdk.org/jdk/pull/18879/files/49a1a58e..22ffae05 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18879=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18879=00-01 Stats: 211 lines in 1 file changed: 0 ins; 211 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18879/head:pull/18879 PR: https://git.openjdk.org/jdk/pull/18879
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v8]
> This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Make Set.of(STRONG) a constant, fix compilation, minor code improvements - Changes: - all: https://git.openjdk.org/jdk/pull/18690/files - new: https://git.openjdk.org/jdk/pull/18690/files/e7cbaaf5..9742f074 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18690=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18690=06-07 Stats: 9 lines in 1 file changed: 4 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v7]
On Wed, 24 Apr 2024 10:01:47 GMT, Claes Redestad wrote: > > I really wish this change was not done with ClassFile API, but with a > > simple bundled ASM, so it would be easily backportable, if we decide to. It > > does not look like CFA buys us a lot here readability/complexity wise: > > [d99b159](https://github.com/openjdk/jdk/commit/d99b1596c5ca57b110c1db88be430c6c54c3d599) > > I would be open to splitting out and pushing the ASM version first and do > this CFA port as a follow-up. That would be good, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2074585421
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v7]
On Wed, 24 Apr 2024 09:57:42 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java > > Co-authored-by: Mandy Chung > I really wish this change was not done with ClassFile API, but with a simple > bundled ASM, so it would be easily backportable, if we decide to. It does not > look like CFA buys us a lot here readability/complexity wise: > [d99b159](https://github.com/openjdk/jdk/commit/d99b1596c5ca57b110c1db88be430c6c54c3d599) I would be open to splitting out and pushing the ASM version first and do this CFA port as a follow-up. - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2074577669
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v7]
On Sun, 14 Apr 2024 14:33:26 GMT, Claes Redestad wrote: >> What are the scenarios which had regressions? >> Given the conservative growth for StringBuilder, it surprises me a bit that >> any scenario would regress. > > I took a second look and it turns out that there were neither regressions nor > improvements in my test, only a few false positives: For C2 the SB chain is > recognized by the (fragile) C2 OptimizeStringConcat pass and transformed into > a shape where the initial size in java bytecode - if any - is ignored. > > With C1 having an initial size can have a significant effect. One way to > provoke a regression there is to have a huge constant suffix while > underestimating the size of the operands, which can lead to excessive > allocation: > > > Name Cnt BaseError TestError > Unit Change > StringConcat.concat23StringConst 5 385,268 ± 5,238341,251 ± 2,606 > ns/op 1,13x (p = 0,000*) > :gc.alloc.rate 6039,095 ± 82,309 12764,169 ± 97,146 > MB/sec 2,11x (p = 0,000*) > :gc.alloc.rate.norm2440,003 ± 0,000 4568,002 ± 0,000 > B/op 1,87x (p = 0,000*) > > > Still a bit better on throughput from less actual copying. > > *If* the `StringBuilder` is sized sufficiently (constants + args * N) things > look much better, of course: > > -XX:TieredStopAtLevel=1 > Name Cnt BaseError TestError > Unit Change > StringConcat.concat23StringConst 5 385,268 ± 5,238 259,628 ± 2,515 > ns/op 1,48x (p = 0,000*) > :gc.alloc.rate 6039,095 ± 82,309 8902,803 ± 86,563 > MB/sec 1,47x (p = 0,000*) > :gc.alloc.rate.norm2440,003 ± 0,000 2424,002 ± 0,000 > B/op 0,99x (p = 0,000*) > > > For most cases having a size based on the sum of the constants plus some > small factor per argument seem to be a decent heuristic - for C1. Since this > adds just one bytecode to the generated method I guess it wouldn't hurt. Presizing this StringBuilder is a thing I looked into once upon a time, discussed here: https://mail.openjdk.org/pipermail/compiler-dev/2015-March/009356.html This work, I understand, the indyfication of string concatenation in the first place. Primitive values can get bounded at appropriate lengths for their types (see e.g. https://stackoverflow.com/a/21146952/869736). It sounds like you're trying to conserve bytecode length, so maybe you don't want to presize the StringBuilder with the exact Object.toString() lengths, though. - PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1576819289
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v7]
> This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/18690/files - new: https://git.openjdk.org/jdk/pull/18690/files/83f4048f..e7cbaaf5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18690=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18690=05-06 Stats: 5 lines in 1 file changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 13:56:32 GMT, Julian Waters wrote: > WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Please mark the PR as draft it is not intended for review. - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2074481887
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v26]
On Sat, 20 Apr 2024 22:31:48 GMT, Scott Gibbons wrote:
>> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64. See
>> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around
>> this change.
>>
>> Overall, making this an intrinsic improves overall performance of
>> `Unsafe::setMemory` by up to 4x for all buffer sizes.
>>
>> Tested with tier-1 (and full CI). I've added a table of the before and
>> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`).
>>
>> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt)
>
> Scott Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 37 commits:
>
> - Merge branch 'openjdk:master' into setMemory
> - Fix UnsafeCopyMemoryMark scope issue
> - Long to short jmp; other cleanup
> - Review comments
> - Address review comments; update copyright years
> - Add enter() and leave(); remove Windows-specific register stuff
> - Fix memory mark after sync to upstream
> - Merge branch 'openjdk:master' into setMemory
> - Set memory test (#23)
>
>* Even more review comments
>
>* Re-write of atomic copy loops
>
>* Change name of UnsafeCopyMemory{,Mark} to UnsafeMemory{Access,Mark}
>
>* Only add a memory mark for byte unaligned fill
>
>* Remove MUSL_LIBC ifdef
>
>* Remove MUSL_LIBC ifdef
> - Set memory test (#22)
>
>* Even more review comments
>
>* Re-write of atomic copy loops
>
>* Change name of UnsafeCopyMemory{,Mark} to UnsafeMemory{Access,Mark}
>
>* Only add a memory mark for byte unaligned fill
> - ... and 27 more: https://git.openjdk.org/jdk/compare/6d569961...1122b500
This introduced a regression, see
[JDK-8331033](https://bugs.openjdk.org/browse/JDK-8331033).
-
PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2074459781
Re: RFR: 8330686: Fix typos in the DatabaseMetaData javadoc [v3]
> Fix typos within the `DatabaseMetaData.java`. Jin Kwon has updated the pull request incrementally with one additional commit since the last revision: 8330686: Fix typos in the DatabaseMetaData javadoc Reviewed-by: jpai - Changes: - all: https://git.openjdk.org/jdk/pull/18860/files - new: https://git.openjdk.org/jdk/pull/18860/files/75f0b111..c8cf9728 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18860=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18860=01-02 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18860.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18860/head:pull/18860 PR: https://git.openjdk.org/jdk/pull/18860
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v12]
On Tue, 23 Apr 2024 20:30:22 GMT, ExE Boss wrote:
>> Evemose has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Added Objects import to sun List
>> - Replaced on-demand import in com.sunList
>> - added non-null assertions
>
> src/java.base/share/classes/java/util/ArrayList.java line 380:
>
>> 378: }
>> 379:
>> 380: int findLastIndexInRange(Predicate filter, int start,
>> int end) {
>
> Suggestion:
>
> private int findLastIndexInRange(Predicate filter, int start,
> int end) {
Yeah i though about it but indexOfRange arent private here so either there is a
point in it or its just legacy without any particular meaning
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18639#discussion_r1577501340
Re: RFR: 8330748: ByteArrayOutputStream.writeTo(OutputStream) pins carrier [v2]
On Tue, 23 Apr 2024 18:08:34 GMT, Brian Burkhalter wrote: >> I was thinking more of a subclass that counted invocations to public methods >> or metering which would cause subclass to double the counts when calling via >> virtual thread. > > So do we think it better not to invoke `toByteArray` here? Using a subclass to count the number of invocations of toByteArray seems a bit strange but in general it is more robust to not rely on a method that may be overridden by a subclass. So I think the suggestion is good. - PR Review Comment: https://git.openjdk.org/jdk/pull/18901#discussion_r1577388594
Re: RFR: 8330686: Fix typos in the DatabaseMetaData javadoc [v2]
On Tue, 23 Apr 2024 09:32:40 GMT, Jin Kwon wrote: >> Fix typos within the `DatabaseMetaData.java`. > > Jin Kwon has updated the pull request incrementally with one additional > commit since the last revision: > > [JDK-8330686] Update copyright year Thank you for the update. The changes look fine to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18860#pullrequestreview-2018924833
