Re: RFR: 8327791: Optimization for new BigDecimal(String) [v18]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen 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 22 additional commits since the
last revision:
- Merge remote-tracking branch 'upstream/master' into optim_dec_new
- Merge remote-tracking branch 'upstream/master' into optim_dec_new
- use while instead for
- Update src/java.base/share/classes/java/math/BigDecimal.java
Co-authored-by: Claes Redestad
- bug fix for CharArraySequence#charAt
- bug fix for CharArraySequence
- fix benchmark
- one CharArraySequence
- restore comment
- easier to compare
- ... and 12 more: https://git.openjdk.org/jdk/compare/1701be36...bb620ba3
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/e516b580..bb620ba3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18177=17
- incr: https://webrevs.openjdk.org/?repo=jdk=18177=16-17
Stats: 6763 lines in 129 files changed: 5031 ins; 1483 del; 249 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Fri, 26 Apr 2024 02:04:38 GMT, Joe Darcy wrote: >>> @cl4es @jddarcy All feedback has been fixed, can it be integrated? >> >> Hello @wenshao , >> >> This change will need additional review from myself or others who maintain >> BigDecimal before it can be integrated. > >> @jddarcy Sorry for the pings, Can you review this PR for me? > > Hi @wenshao , > Can you provide some additional context about the benefits of this change > beyond the micro/nano bechmark results that have been discussed. For example, > is there interesting workload the change improves, etc.? Thanks. Thanks @jddarcy, I will provide new performance test data tomorrow (Saturday, April 26th) - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2078625327
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote:
>> Splitting out the ASM-based version from #18690 to push that first under the
>> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on
>> this as a subtask. See discussion in that #18690 for more details,
>> discussion and motivation for this.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove accidental use of java.lang.classfile
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1057:
> 1055: */
> 1056: private static final class SimpleStringBuilderStrategy {
> 1057: static final int CLASSFILE_VERSION = 52; // JDK 8
Alternatively, this can use `ClassFileFormatVersion.latest().major()` which
exists since JDK 20.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580432036
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote: >> Splitting out the ASM-based version from #18690 to push that first under the >> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on >> this as a subtask. See discussion in that #18690 for more details, >> discussion and motivation for this. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidental use of java.lang.classfile Also a side note for backport: ClassFileDumper exists only since JDK 21, so for earlier backports you need to use an alternative dumping method or remove dumping ability. - PR Comment: https://git.openjdk.org/jdk/pull/18953#issuecomment-2078532971
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v9]
On Wed, 24 Apr 2024 19:13:43 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: > > Nits, clean up constant, introduce missing constant MethodTypeDesc for > toString It's a little confusing for JDK-8327247 to have 2 PRs but it's okay. We can add a note in JDK-8327247 to clarify. I assume you plan to use PR to convert to use ClassFile API after you pull from JDK-8327247 once integrated? - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2078512445
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote: >> Splitting out the ASM-based version from #18690 to push that first under the >> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on >> this as a subtask. See discussion in that #18690 for more details, >> discussion and motivation for this. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidental use of java.lang.classfile Nit: the copyright header needs update before integration. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18953#pullrequestreview-2023964506
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 19 Mar 2024 19:32:10 GMT, Joe Darcy wrote: >> I think splitting `CharArraySequence` into two versions is somewhat dubious >> as more observable types at call sites may mean the performance gain in >> targeted micros is lost. How much of an improvement did you observe from >> this? Again the `char[]` constructors is probably less performance sensitive >> than the others. > >> @cl4es @jddarcy All feedback has been fixed, can it be integrated? > > Hello @wenshao , > > This change will need additional review from myself or others who maintain > BigDecimal before it can be integrated. > @jddarcy Sorry for the pings, Can you review this PR for me? Hi @wenshao , Can you provide some additional context about the benefits of this change beyond the micro/nano bechmark results that have been discussed. For example, is there interesting workload the change improves, etc.? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2078497794
Re: RFR: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute [v2]
> Remove unnecessary setting of variable y, found by an IDE inspection noted in > the bug report. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update copyright year. - Changes: - all: https://git.openjdk.org/jdk/pull/18963/files - new: https://git.openjdk.org/jdk/pull/18963/files/e8f5c334..f0ed7d5f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18963=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18963=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18963.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18963/head:pull/18963 PR: https://git.openjdk.org/jdk/pull/18963
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v9]
On Thu, 25 Apr 2024 23:24:07 GMT, Jonathan Gibbons wrote:
>> Please review the updates to support a proposed new
>> `-Xlint:dangling-doc-comments` option.
>>
>> The work can be thought of as in 3 parts:
>>
>> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
>> is possible to specify the appropriately configured `Lint` object when it is
>> time to consider whether to generate the diagnostic or not.
>>
>> 2. Updates to the `javac` front end to record "dangling docs comments" found
>> near the beginning of a declaration, and to report them using an instance of
>> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
>> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
>> procedure for handling dangling doc comments is described in this comment in
>> `JavacParser`.
>>
>> * Dangling documentation comments are handled as follows.
>> * 1. {@code Scanner} adds all doc comments to a queue of
>> * recent doc comments. The queue is flushed whenever
>> * it is known that the recent doc comments should be
>> * ignored and should not cause any warnings.
>> * 2. The primary documentation comment is the one obtained
>> * from the first token of any declaration.
>> * (using {@code token.getDocComment()}.
>> * 3. At the end of the "signature" of the declaration
>> * (that is, before any initialization or body for the
>> * declaration) any other "recent" comments are saved
>> * in a map using the primary comment as a key,
>> * using this method, {@code saveDanglingComments}.
>> * 4. When the tree node for the declaration is finally
>> * available, and the primary comment, if any,
>> * is "attached", (in {@link #attach}) any related
>> * dangling comments are also attached to the tree node
>> * by registering them using the {@link #deferredLintHandler}.
>> * 5. (Later) Warnings may be genereated for the dangling
>> * comments, subject to the {@code -Xlint} and
>> * {@code @SuppressWarnings}.
>>
>>
>> 3. Updates to the make files to disable the warnings in modules for which
>> the
>> warning is generated. This is often because of the confusing use of `/**` to
>> create box or other standout comments.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> revert need to disable warning
looks good
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
583:
> 581: * dangling comments are also attached to the tree node
> 582: * by registering them using the {@link #deferredLintHandler}.
> 583: * 5. (Later) Warnings may be genereated for the dangling
typo: generated
-
Marked as reviewed by vromero (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18527#pullrequestreview-2023792395
PR Review Comment: https://git.openjdk.org/jdk/pull/18527#discussion_r1580263826
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v27]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: add memviz bullet for finalization - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/0b9d3efc..77d53818 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=26 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=25-26 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 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: 8303689: javac -Xlint could/should report on "dangling" doc comments [v9]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
revert need to disable warning
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/16a265a2..39689a52
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=08
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=07-08
Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v26]
> 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: remove quotes from dequeue - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/16fcc764..0b9d3efc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=25 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=24-25 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: 8314480: Memory ordering spec updates in java.lang.ref [v25]
On Thu, 25 Apr 2024 08:25:39 GMT, Viktor Klang wrote:
>> 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
>
> src/java.base/share/classes/java/lang/ref/package-info.java line 153:
>
>> 151: * The enqueueing of a reference (by the garbage collector, or
>> 152: * by a successful call to {@link Reference#enqueue})
>> happens-before
>> 153: * the reference is removed from the queue (dequeued).
>
> @bchristi-git I wonder if it makes sense to use the same style when referring
> to "dequeue", so either always within quotes or only using em (see
> line 108)
Sure; I'll remove the quotes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1580238810
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v3]
On Thu, 25 Apr 2024 22:26:04 GMT, Claes Redestad wrote: >> When analyzing (startup) performance of the Classfile API I found this >> opportunity to further improve `MethodTypeDescImpl::descriptorString`. >> >> Performance improves across the board in existing microbenchmarks: >> >> Name >> (descString) Cnt Base ErrorTest Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 >> ns/op 1,68x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 >> ns/op 2,11x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = >> 0,000*) >> * = significant >> >> >> The improvement is even more pronounced when running with `-Xint`, which is >> relevant for reducing startup overheads of early ClassFile API use: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± >> 101,466 ns/op 1,95x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± >> 41,892 ns/op 2,36x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op >> 2,46x (p = 0,000*) >> * = significant >> ``` >> >> I also applied similar approach to `MethodTypeDesc::displayDescriptor`: >> while not performance sensitive I think these are so inter-related that it >> makes sense to implement them in a similar fashion. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Calculate length precisely For the precise-length approach, do you have the results? I guess if we can avoid copying the stringbuilder array, we can make this even faster. - PR Comment: https://git.openjdk.org/jdk/pull/18945#issuecomment-2078311772
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v3]
On Thu, 25 Apr 2024 22:57:50 GMT, Florent Guillaume wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Calculate length precisely
>
> src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java line
> 181:
>
>> 179: sb.append(argType.descriptorString());
>> 180: }
>> 181: desc =
>> sb.append(')').append(returnType().descriptorString()).toString();
>
> Nit: the rest of the code (and even the new sizing part you added) uses
> `returnType` instead of `returnType()`
Think the `returnType()` call was for parity with displayDescriptor changes;
now that we are just in the impl class, either way is fine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18945#discussion_r1580235742
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v3]
On Thu, 25 Apr 2024 22:26:04 GMT, Claes Redestad wrote:
>> When analyzing (startup) performance of the Classfile API I found this
>> opportunity to further improve `MethodTypeDescImpl::descriptorString`.
>>
>> Performance improves across the board in existing microbenchmarks:
>>
>> Name
>> (descString) Cnt Base ErrorTest Error Unit Change
>> MethodTypeDescFactories.descriptorString
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189
>> ns/op 1,68x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636
>> ns/op 2,11x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p =
>> 0,000*)
>> * = significant
>>
>>
>> The improvement is even more pronounced when running with `-Xint`, which is
>> relevant for reducing startup overheads of early ClassFile API use:
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> MethodTypeDescFactories.descriptorString
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ±
>> 101,466 ns/op 1,95x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ±
>> 41,892 ns/op 2,36x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op
>> 2,46x (p = 0,000*)
>> * = significant
>> ```
>>
>> I also applied similar approach to `MethodTypeDesc::displayDescriptor`:
>> while not performance sensitive I think these are so inter-related that it
>> makes sense to implement them in a similar fashion.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Calculate length precisely
src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java line 181:
> 179: sb.append(argType.descriptorString());
> 180: }
> 181: desc =
> sb.append(')').append(returnType().descriptorString()).toString();
Nit: the rest of the code (and even the new sizing part you added) uses
`returnType` instead of `returnType()`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18945#discussion_r1580229759
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v8]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 11 commits:
- Merge remote-tracking branch 'upstream/master' into
8303689.dangling-comments # Please enter a commit message to explain why this
merge is necessary, # especially if it merges an updated upstream into a topic
branch. # # Lines starting with '#' will be ignored, and an empty message
aborts # the
commit.
- Merge with upstream/master
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- ... and 1 more: https://git.openjdk.org/jdk/compare/1c238d43...16a265a2
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18527=07
Stats: 485 lines in 59 files changed: 387 ins; 3 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
On Thu, 25 Apr 2024 21:32:03 GMT, Joe Darcy wrote: > Remove unnecessary setting of variable y, found by an IDE inspection noted in > the bug report. Marked as reviewed by bpb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18963#pullrequestreview-2023708504
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v3]
> When analyzing (startup) performance of the Classfile API I found this > opportunity to further improve `MethodTypeDescImpl::descriptorString`. > > Performance improves across the board in existing microbenchmarks: > > Name > (descString) Cnt Base ErrorTest Error Unit Change > MethodTypeDescFactories.descriptorString > (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 > ns/op 1,68x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 > ns/op 2,11x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = > 0,000*) > * = significant > > > The improvement is even more pronounced when running with `-Xint`, which is > relevant for reducing startup overheads of early ClassFile API use: > > Name > (descString) Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.descriptorString > (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± > 101,466 ns/op 1,95x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± > 41,892 ns/op 2,36x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op > 2,46x (p = 0,000*) > * = significant > ``` > > I also applied similar approach to `MethodTypeDesc::displayDescriptor`: > while not performance sensitive I think these are so inter-related that it > makes sense to implement them in a similar fashion. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Calculate length precisely - Changes: - all: https://git.openjdk.org/jdk/pull/18945/files - new: https://git.openjdk.org/jdk/pull/18945/files/2979847c..15c8d39c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18945=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18945=01-02 Stats: 22 lines in 3 files changed: 8 ins; 4 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18945.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18945/head:pull/18945 PR: https://git.openjdk.org/jdk/pull/18945
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 13:34:50 GMT, Claes Redestad wrote: >> When analyzing (startup) performance of the Classfile API I found this >> opportunity to further improve `MethodTypeDescImpl::descriptorString`. >> >> Performance improves across the board in existing microbenchmarks: >> >> Name >> (descString) Cnt Base ErrorTest Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 >> ns/op 1,68x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 >> ns/op 2,11x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = >> 0,000*) >> * = significant >> >> >> The improvement is even more pronounced when running with `-Xint`, which is >> relevant for reducing startup overheads of early ClassFile API use: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± >> 101,466 ns/op 1,95x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± >> 41,892 ns/op 2,36x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op >> 2,46x (p = 0,000*) >> * = significant >> ``` >> >> I also applied similar approach to `MethodTypeDesc::displayDescriptor`: >> while not performance sensitive I think these are so inter-related that it >> makes sense to implement them in a similar fashion. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > comma-separated Correction: I ran the wrong benchmark for that long descriptor test! 臘 On the `descriptorString` benchmark there _is_ a regression with my patch on longer descriptors, -30% on that example. Since the strings are all readily available pre-calculating the size is cheap enough to make sense. This makes the patch more or less neutral on large descriptors, extends the gain on few-arg methods, but reduces the win on cases where there are no args (due an extra branch et.c.). On balance this seems like an improvement: Name (descString) CntBase ErrorTestError Unit Change MethodTypeDescFactories.descriptorString (Ljava/lang/Object;Ljava/lang/String;)I 6 43,180 ± 0,575 30,547 ± 0,387 ns/op 1,41x (p = 0,000*) MethodTypeDescFactories.descriptorString ()V 6 20,717 ± 0,128 17,233 ± 0,242 ns/op 1,20x (p = 0,000*) MethodTypeDescFactories.descriptorString ([IJLjava/lang/String;Z)Ljava/util/List; 6 89,834 ± 1,951 40,149 ± 0,326 ns/op 2,24x (p = 0,000*) MethodTypeDescFactories.descriptorString ()[Ljava/lang/String; 6 21,438 ± 0,490 15,937 ± 0,263 ns/op 1,35x (p = 0,000*) MethodTypeDescFactories.descriptorString (..IIJ)V 6 52,429 ± 0,117 49,396 ± 0,153 ns/op 1,06x (p = 0,000*) MethodTypeDescFactories.descriptorString (.). 6 178,205 ± 0,518 176,856 ± 5,577 ns/op 1,01x (p = 0,158 ) * = significant `.` is illegal in descriptor strings I used this as wildcard to be replaced with the descriptor string for the benchmark class as a ways to produce very large descriptors. Since this strays a bit from what's doable in `MethodTypeDesc::displayDescriptor` I'm reverting those changes and focus this PR on the relevant piece of machinery. - PR Comment: https://git.openjdk.org/jdk/pull/18945#issuecomment-2078267325
Re: RFR: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
On Thu, 25 Apr 2024 21:32:03 GMT, Joe Darcy wrote: > Remove unnecessary setting of variable y, found by an IDE inspection noted in > the bug report. Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18963#pullrequestreview-2023687212
RFR: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
Remove unnecessary setting of variable y, found by an IDE inspection noted in the bug report. - Commit messages: - JDK-8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute Changes: https://git.openjdk.org/jdk/pull/18963/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18963=00 Issue: https://bugs.openjdk.org/browse/JDK-8331108 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18963.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18963/head:pull/18963 PR: https://git.openjdk.org/jdk/pull/18963
Re: RFR: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
On Thu, 25 Apr 2024 21:32:03 GMT, Joe Darcy wrote: > Remove unnecessary setting of variable y, found by an IDE inspection noted in > the bug report. All Math and StrictMath regression tests pass with this change. Examining the code, y does look to be overwritten on all the code paths where it factors into the returned result. - PR Comment: https://git.openjdk.org/jdk/pull/18963#issuecomment-2078210519
Re: Generalizing binary search
On Thu, Apr 25, 2024 at 2:09 PM Pavel Rappo wrote: > > > > On 25 Apr 2024, at 19:41, David Lloyd wrote: > > > > The JDK contains a few collection- and array-oriented implementations of > binary search. For the most part, they all work the same way: search the > target "thing" by index using the obvious bisection strategy, returning > either /index/ or /-index-1/ depending on whether it was found at the end > of the search. > > > > However, if you're doing a binary search on a thing that is not a list > or an array, you have two choices: try to make the thing you're searching > on implement List (often infeasible) or write your own binary search. > > > > I'm really tired of writing my own binary search. I've probably done it > 50 times by now, each one using a slightly different access and/or compare > strategy, and every time is a new chance to typo something or get something > backwards, leading to irritating debugging sessions and higher dentist > bills. > > Can we safely say that it sets your teeth on edge? > You could say that. :) Have a look at this recently filed issue: > https://bugs.openjdk.org/browse/JDK-8326330 Oh, nice, I didn't see that come across. It's even more general than my version: I like that. My only complaint is (once again) that the lambda must be capturing in order to work with a plain `IntPredicate`/`LongPredicate`. -- - DML • he/him
Re: In support of Instant.minus(Instant)
java.time.* already has the `until(ChronoLocalDate)` method on LocalDate. It would be reasonable to add a similar method to various other classes. This potentially gives you Duration dur = start.until(end) I'm wary of adding the opposite (given until() is already there). I'm particularly wary of using minus() as the verb for the opposite as minus() means something different in other parts of the API (minus() is used to subtract a TemporalAmounrt, not a Temporal). Stephen On Thu, 25 Apr 2024 at 19:57, Kurt Alfred Kluever wrote: > > Hi core-libs-dev, > > The java.time API already supports subtracting two Instants (end - start) via > Duration.between(Temporal, Temporal), but we've found the parameter ordering > (which requires a bit of "mental gymnastics") and API location to be a bit > unnatural. > > Parameter Ordering > > We very often see folks write code like this: Duration elapsed = > Duration.ofMillis(end.toEpochMilli() - start.toEpochMilli()); > > This closely matches the mental model of what they're trying to accomplish, > but it is longer (and more complex) than it needs to be, it drops > sub-millisecond precision, and it requires decomposing the java.time types > (which we strongly discourage). If you want to "simplify" the above > statement, you must remember to swap the argument order: Duration elapsed = > Duration.between(start, end); > > Many of us find representing (end - start) as between(start, end) to be > confusing. > > API Location > > We do not believe Duration is the most obvious place to find this method; if > you want a way to subtract two Instant values, an instance method on Instant > is a more obvious place to look. Pushing what could be an instance method to > a static utility method feels unnatural. > > JDK-8276624 (https://bugs.openjdk.org/browse/JDK-8276624) proposes to add > Temporal.minus(Temporal) as a default method (which would effectively > accomplish the same thing), but we do not recommend implementing that > proposal as specified. A default method on Temporal would require runtime > exceptions to be thrown from other Temporal types like LocalDate or Year. It > would also allow oddities like instant.minus(year) to compile (but presumably > throw at runtime). Conceptually, this API would not make sense for certain > types (e.g., LocalDate — the difference between two LocalDates is a Period, > not a Duration). > > Instead, we recommend adding a new instance method: instant.minus(instant) > (which returns a Duration), and possibly also adding > localDate.minus(localDate) (which returns a Period). However note that we've > seen a lot of confusion using the Period API (but that's a separate > discussion). > > While we generally don't like having 2 public APIs that accomplish the same > thing, in this case we feel the discoverability and simplicity of the new > API(s) outweighs the cost of an additional public API. > > Please consider this a +1 from our team to add instant.minus(instant). > > Regards, > > -Kurt Alfred Kluever (k...@google.com) > (on behalf of Google's Java and Kotlin Ecosystem Team, aka the Guava team)
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 Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18913#pullrequestreview-2023498303
Re: RFR: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
On Thu, 25 Apr 2024 00:51:41 GMT, Adam Sotona wrote: > 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. Ah yes, i missed that. It could be two tags, a lower and upper bound, because TAG_FIELDREF, TAG_METHODREF, and TAG_INTERFACEMETHODREF are consecutive values (9 to 11). - PR Comment: https://git.openjdk.org/jdk/pull/18907#issuecomment-2078099914
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 16:46:25 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove accidental use of java.lang.classfile
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line
> 1059:
>
>> 1057: */
>> 1058: private static final class SimpleStringBuilderStrategy {
>> 1059: static final int CLASSFILE_VERSION =
>> ClassFile.latestMajorVersion();
>
> Still breaks backward ASM port, we should use a fixed version like 52 for
> JAVA_8 and convert to latest only in the CF conversion later.
Good catch. In the code I am
[reviving](https://github.com/cl4es/jdk/commit/36c4b11bc6cf5a008d5935934aa75f2d2bbe6a23#diff-1339c269a3729d849799d29a7431ccd508a034ced91c1796b952795396843891L771)
this field was simply set to `52`. Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580047931
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
> Splitting out the ASM-based version from #18690 to push that first under the > JBS (to help backporting). Keeping #18690 open to rebase and follow-up on > this as a subtask. See discussion in that #18690 for more details, discussion > and motivation for this. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove accidental use of java.lang.classfile - Changes: - all: https://git.openjdk.org/jdk/pull/18953/files - new: https://git.openjdk.org/jdk/pull/18953/files/d8d5e5d3..a4dfbfca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18953=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18953=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18953.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18953/head:pull/18953 PR: https://git.openjdk.org/jdk/pull/18953
Re: Generalizing binary search
> On 25 Apr 2024, at 19:41, David Lloyd wrote:
>
> The JDK contains a few collection- and array-oriented implementations of
> binary search. For the most part, they all work the same way: search the
> target "thing" by index using the obvious bisection strategy, returning
> either /index/ or /-index-1/ depending on whether it was found at the end of
> the search.
>
> However, if you're doing a binary search on a thing that is not a list or an
> array, you have two choices: try to make the thing you're searching on
> implement List (often infeasible) or write your own binary search.
>
> I'm really tired of writing my own binary search. I've probably done it 50
> times by now, each one using a slightly different access and/or compare
> strategy, and every time is a new chance to typo something or get something
> backwards, leading to irritating debugging sessions and higher dentist bills.
Can we safely say that it sets your teeth on edge?
> It got me to thinking that it wouldn't be too hard to make a "general" binary
> search which can search on anything, so that's what I did. I was thinking
> that it might be interesting to try adding this into the JDK somehow.
>
> This implementation is more or less what I now copy & paste to different
> projects at the moment:
>
> public static int binarySearch(C collection, int start, int end, T
> key, Comparator cmp, IntGetter getter) {
> int low = start;
> int high = end - 1;
> while (low <= high) {
> int mid = low + high >>> 1;
> int res = cmp.compare(getter.get(collection, mid), key);
> if (res < 0) {
> low = mid + 1;
> } else if (res > 0) {
> high = mid - 1;
> } else {
> return mid;
> }
> }
> return -low - 1;
> }
> // (Plus variations for `Comparable` keys and long indices)
>
> A key problem with this approach is that in the JDK, there is no
> `ObjIntFunction` or `ObjLongFunction` which would be necessary to
> implement the "getter" portion of the algorithm (despite the existence of the
> analogous `ObjIntConsumer` and `ObjLongConsumer`). So, I also have to
> copy/paste a `IntGetter`/`LongGetter` as well, which is annoying.
>
> A slightly less-good approach is for the general `binarySearch` method to
> accept a `IntFunction`/`LongFunction`, and drop the `C collection`
> parameter, like this:
>
> public static int binarySearch(int start, int end, T key,
> Comparator cmp, IntFunction getter) { ... }
>
> In this case, we can use the function types that the JDK already provides,
> but we would very likely have to also create a capturing lambda (e.g.
> `myList::get` instead of `List::get`). Maybe this isn't that bad of a
> compromise.
>
> It would be possible to replace the existing `binarySearch` implementations
> with delegating calls to a generalized implementation. For `Collections`, the
> indexed version uses `List::get` and the iterator version uses a helper
> lambda to move the iterator and get the result. For arrays, a lambda would be
> provided which gets the corresponding array element. If the non-capturing
> variant is used, then (on paper at least) this version should perform
> similarly to the existing implementations, with less code needed overall.
> However, if a capturing lambda is required (due to the aforementioned lack of
> `ObjXFunction`), then this could be slightly worse-performing than the
> existing implementation due to the construction (and maybe dispatch) overhead
> of the lambda. Some real-world benchmarks would have to be written with
> various-sized data sets.
>
> It would also be possible to produce primitive variations which operate on
> int, float, long, and double values, using existing functions if capturing is
> deemed "OK". It is also possible to produce a variation which uses a `long`
> for the index, for huge data sets (for example, very large mapped files using
> `MemorySegment`).
>
> Also unclear is: where would it live? `Collections`? Somewhere else?
>
> Any thoughts/opinions would be appreciated (even if they are along the lines
> of "it's not worth the effort"). Particularly, any insight would be
> appreciated as to whether or not this kind of hypothetical enhancement would
> warrant a JEP (I wouldn't think so, but I'm no expert at such assessments).
>
> --
> - DML • he/him
Have a look at this recently filed issue:
https://bugs.openjdk.org/browse/JDK-8326330
-Pavel
In support of Instant.minus(Instant)
Hi core-libs-dev, The java.time API already supports subtracting two Instants (end - start) via Duration.between(Temporal, Temporal), but we've found the parameter ordering (which requires a bit of "mental gymnastics") and API location to be a bit unnatural. Parameter Ordering We very often see folks write code like this: Duration elapsed = Duration.ofMillis(end.toEpochMilli() - start.toEpochMilli()); This closely matches the mental model of what they're trying to accomplish, but it is longer (and more complex) than it needs to be, it drops sub-millisecond precision, and it requires decomposing the java.time types (which we strongly discourage). If you want to "simplify" the above statement, you must remember to swap the argument order: Duration elapsed = Duration.between(start, end); Many of us find representing (end - start) as between(start, end) to be confusing. API Location We do not believe Duration is the most obvious place to find this method; if you want a way to subtract two Instant values, an instance method on Instant is a more obvious place to look. Pushing what could be an instance method to a static utility method feels unnatural. JDK-8276624 (https://bugs.openjdk.org/browse/JDK-8276624) proposes to add Temporal.minus(Temporal) as a default method (which would effectively accomplish the same thing), but we do not recommend implementing that proposal as specified. A default method on Temporal would require runtime exceptions to be thrown from other Temporal types like LocalDate or Year. It would also allow oddities like instant.minus(year) to compile (but presumably throw at runtime). Conceptually, this API would not make sense for certain types (e.g., LocalDate — the difference between two LocalDates is a Period, not a Duration). Instead, we recommend adding a new instance method: instant.minus(instant) (which returns a Duration), and possibly also adding localDate.minus(localDate) (which returns a Period). However note that we've seen a lot of confusion using the Period API (but that's a separate discussion). While we generally don't like having 2 public APIs that accomplish the same thing, in this case we feel the discoverability and simplicity of the new API(s) outweighs the cost of an additional public API. Please consider this a +1 from our team to add instant.minus(instant). Regards, -Kurt Alfred Kluever (k...@google.com) (on behalf of Google's Java and Kotlin Ecosystem Team, aka the Guava team)
Generalizing binary search
The JDK contains a few collection- and array-oriented implementations of
binary search. For the most part, they all work the same way: search the
target "thing" by index using the obvious bisection strategy, returning
either /index/ or /-index-1/ depending on whether it was found at the end
of the search.
However, if you're doing a binary search on a thing that is not a list or
an array, you have two choices: try to make the thing you're searching on
implement List (often infeasible) or write your own binary search.
I'm really tired of writing my own binary search. I've probably done it 50
times by now, each one using a slightly different access and/or compare
strategy, and every time is a new chance to typo something or get something
backwards, leading to irritating debugging sessions and higher
dentist bills.
It got me to thinking that it wouldn't be too hard to make a "general"
binary search which can search on anything, so that's what I did. I was
thinking that it might be interesting to try adding this into the JDK
somehow.
This implementation is more or less what I now copy & paste to different
projects at the moment:
public static int binarySearch(C collection, int start, int end,
T key, Comparator cmp, IntGetter getter) {
int low = start;
int high = end - 1;
while (low <= high) {
int mid = low + high >>> 1;
int res = cmp.compare(getter.get(collection, mid), key);
if (res < 0) {
low = mid + 1;
} else if (res > 0) {
high = mid - 1;
} else {
return mid;
}
}
return -low - 1;
}
// (Plus variations for `Comparable` keys and long indices)
A key problem with this approach is that in the JDK, there is no
`ObjIntFunction` or `ObjLongFunction` which would be necessary
to implement the "getter" portion of the algorithm (despite the existence
of the analogous `ObjIntConsumer` and `ObjLongConsumer`). So, I also
have to copy/paste a `IntGetter`/`LongGetter` as well, which is
annoying.
A slightly less-good approach is for the general `binarySearch` method to
accept a `IntFunction`/`LongFunction`, and drop the `C collection`
parameter, like this:
public static int binarySearch(int start, int end, T key,
Comparator cmp, IntFunction getter) { ... }
In this case, we can use the function types that the JDK already provides,
but we would very likely have to also create a capturing lambda (e.g.
`myList::get` instead of `List::get`). Maybe this isn't that bad of a
compromise.
It would be possible to replace the existing `binarySearch` implementations
with delegating calls to a generalized implementation. For `Collections`,
the indexed version uses `List::get` and the iterator version uses a helper
lambda to move the iterator and get the result. For arrays, a lambda would
be provided which gets the corresponding array element. If the
non-capturing variant is used, then (on paper at least) this version should
perform similarly to the existing implementations, with less code needed
overall. However, if a capturing lambda is required (due to the
aforementioned lack of `ObjXFunction`), then this could be slightly
worse-performing than the existing implementation due to the construction
(and maybe dispatch) overhead of the lambda. Some real-world benchmarks
would have to be written with various-sized data sets.
It would also be possible to produce primitive variations which operate on
int, float, long, and double values, using existing functions if capturing
is deemed "OK". It is also possible to produce a variation which uses a
`long` for the index, for huge data sets (for example, very large mapped
files using `MemorySegment`).
Also unclear is: where would it live? `Collections`? Somewhere else?
Any thoughts/opinions would be appreciated (even if they are along the
lines of "it's not worth the effort"). Particularly, any insight would be
appreciated as to whether or not this kind of hypothetical enhancement
would warrant a JEP (I wouldn't think so, but I'm no expert at such
assessments).
--
- DML • he/him
Re: Usage of iconv()
On 4/24/24 4:24 AM, Magnus Ihse Bursie wrote: That is a good question. libiconv is used only on macOS and AIX, for a few libraries, as you say. I just tried removing -liconv from the macOS dependencies and recompiled, just to see what would happen. There were three instances for macOS: libsplashscreen, libjdwp and libinstrument. libsplashscreen uses it in splashscreen_sys.m, where it is used to convert the jar file name. This is called from the launcher, in java.base/share/native/libjli/java.c libjdwp uses it in utf_util.c, where it is used to convert file name and command lines, iiuc. It is likely that we have similar (but better) ways to convert charsets elsewhere in our libraries that can be used instead of libiconv. I guess these are not the only two places where a filename must be converted from the platform charset to UTF8. So whatever replacement there might be, it needs to be something that is available very early in the life of the VM, in fact before there is a VM running. -phil.
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
On Thu, 25 Apr 2024 14:15:56 GMT, Claes Redestad wrote:
> Splitting out the ASM-based version from #18690 to push that first under the
> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on
> this as a subtask. See discussion in that #18690 for more details, discussion
> and motivation for this.
Only with ASM can we realize how concise ClassFile API is!
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1059:
> 1057: */
> 1058: private static final class SimpleStringBuilderStrategy {
> 1059: static final int CLASSFILE_VERSION =
> ClassFile.latestMajorVersion();
Still breaks backward ASM port, we should use a fixed version like 52 for
JAVA_8 and convert to latest only in the CF conversion later.
-
PR Review: https://git.openjdk.org/jdk/pull/18953#pullrequestreview-2023078249
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1579822765
Integrated: 8319990: Update CLDR to Version 45.0
On Mon, 22 Apr 2024 18:44:33 GMT, Naoto Sato wrote: > Upgrading the CLDR to version 45.0. We now have versions specified in the > javadoc (`LocaleServiceProvider`), a corresponding CSR has also been drafted. This pull request has now been integrated. Changeset: 1c238d43 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/1c238d43e81acf516297f26660059d0bab5b5b8a Stats: 7401 lines in 1075 files changed: 307 ins; 4640 del; 2454 mod 8319990: Update CLDR to Version 45.0 Reviewed-by: joehw, jlu - PR: https://git.openjdk.org/jdk/pull/18900
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 13:34:50 GMT, Claes Redestad wrote: >> When analyzing (startup) performance of the Classfile API I found this >> opportunity to further improve `MethodTypeDescImpl::descriptorString`. >> >> Performance improves across the board in existing microbenchmarks: >> >> Name >> (descString) Cnt Base ErrorTest Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 >> ns/op 1,68x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 >> ns/op 2,11x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = >> 0,000*) >> * = significant >> >> >> The improvement is even more pronounced when running with `-Xint`, which is >> relevant for reducing startup overheads of early ClassFile API use: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± >> 101,466 ns/op 1,95x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± >> 41,892 ns/op 2,36x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op >> 2,46x (p = 0,000*) >> * = significant >> ``` >> >> I also applied similar approach to `MethodTypeDesc::displayDescriptor`: >> while not performance sensitive I think these are so inter-related that it >> makes sense to implement them in a similar fashion. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > comma-separated What if we replace `24` with a precalculated value: int size = 2 + returnType().descriptorString().length(); for (var param : argTypes) size += param.descriptorString().length(); (Would be even better if we can just trust the internal array to avoid copy allocation) - PR Comment: https://git.openjdk.org/jdk/pull/18945#issuecomment-2077600166
Integrated: 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 This pull request has now been integrated. Changeset: 5af6b45e Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/5af6b45eefd227e3e046ca22a404ae8a23174160 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8330615: avoid signed integer overflows in zip_util.c readCen / hashN Reviewed-by: lucy, mdoerr - PR: https://git.openjdk.org/jdk/pull/18908
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 Hello Matthias, the tests completed a couple of hours back and no failures related to this change have been observed. Thank you for waiting. - PR Comment: https://git.openjdk.org/jdk/pull/18908#issuecomment-2077527559
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v5]
> 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: whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/18583/files - new: https://git.openjdk.org/jdk/pull/18583/files/c93a71f0..a1984501 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18583=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18583=03-04 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 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
Integrated: 8329805: Deprecate for removal ObjectOutputStream.PutField.write
On Tue, 16 Apr 2024 19:44:36 GMT, Roger Riggs wrote: > The method `java.io.ObjectOutputStream.PutField.write` has been deprecated > since 1.4 and should be deprecated for removal. The Deprecation annotation is > updated to indicate the intention to remov the method. This pull request has now been integrated. Changeset: 4dfaa9b5 Author:Roger Riggs URL: https://git.openjdk.org/jdk/commit/4dfaa9b5bd4f9733e5a67d7c5b55eaa5ad4e27e4 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8329805: Deprecate for removal ObjectOutputStream.PutField.write Reviewed-by: naoto, iris - PR: https://git.openjdk.org/jdk/pull/18802
Re: RFR: 8330624: Inconsistent use of semicolon in the code snippet of java.io.Serializable javadoc
On Sat, 20 Apr 2024 11:49:30 GMT, Korov wrote: > Fix the description of java.io.Serializable. LGTM; trivial fix of example javadoc code - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18874#pullrequestreview-2022703632
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 13:35:59 GMT, Chen Liang wrote: > Do we have any research on the average length of method descriptor strings? I > wonder if manual pre-allocation works better (iterating all descriptor > strings, allocate the sum of their sizes plus 2 (for parentheses), as > descriptor strings won't be re-calculated after initial allocation.) > Especially in case of user code, as many user classes have very long package > names that can easily make the string much longer than the 24-char default. I don't know of any systematic research on this, but in the code generators we have in the JDK short method signatures outweigh more complex ones by far. So if we can improve for small descriptors without regressing large ones then that's a pretty good win. I've done some micro-benchmarking on longer descriptors and there's still a gain from the proposed patch: MethodTypeDescFactories.ofDesc -p descString=(Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;)Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories; Name CntBaseError Test Error Unit Change MethodTypeDescFactories.ofDescriptor 6 442,112 ± 16,175 423,926 ± 6,245 ns/op 1,04x (p = 0,000*) * = significant I'd be happy to add a variant that stresses larger descriptors. - PR Comment: https://git.openjdk.org/jdk/pull/18945#issuecomment-2077348963
Re: RFR: 8326951: Missing `@since` tags [v7]
> I added `@since` tags for methods/constructors that do not match the `@since` > of the enclosing class. > > The `write` method already existed in `PrintStream` in earlier versions and > instances of it could always call this method, since it extends > `FilterOutputStream` [which has the > method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96). > > This is similar to #18032 and #18373 > > For context, I am writing tests to check for accurate use of `@since` tags in > documentation comments in source code. > We're following these rules for now: > > ### Rule 1: Introduction of New Elements > > - If an element is new in JDK N, with no equivalent in JDK N-1, it must > include `@since N`. > - Exception: Member elements (fields, methods, nested classes) may omit > `@since` if their version matches the value specified for the enclosing class > or interface. > > ### Rule 2: Existing Elements in Subsequent JDK Versions > > - If an element exists in JDK N, with an equivalent in JDK N-1, it should not > include `@since N`. > > ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since` > > - When inspecting methods, prioritize the `@since` annotation of the > supertype's overridden method. > - If unavailable or if the enclosing class's `@since` is newer, use the > enclosing element's `@since`. > > I.e. if A extends B, and we add a method to B in JDK N, and add an override > of the method to A in JDK M (M > N), we will use N as the effective `@since` > for the method. Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision: deal with methods with different return types - added some spaces for readability - Changes: - all: https://git.openjdk.org/jdk/pull/18055/files - new: https://git.openjdk.org/jdk/pull/18055/files/390a21f9..670acaec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18055=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18055=05-06 Stats: 13 lines in 5 files changed: 12 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18055.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18055/head:pull/18055 PR: https://git.openjdk.org/jdk/pull/18055
RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
Splitting out the ASM-based version from #18690 to push that first under the JBS (to help backporting). Keeping #18690 open to rebase and follow-up on this as a subtask. See discussion in that #18690 for more details, discussion and motivation for this. - Commit messages: - Adapt main PR feedback to ASM version - ASM fallback version Changes: https://git.openjdk.org/jdk/pull/18953/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18953=00 Issue: https://bugs.openjdk.org/browse/JDK-8327247 Stats: 300 lines in 2 files changed: 294 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18953.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18953/head:pull/18953 PR: https://git.openjdk.org/jdk/pull/18953
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:05:27 GMT, Aleksey Shipilev 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. > >> > 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. @shipilev @mlchung opened new PR #18953 for pushing the ASM-based version. Adapted applicable code changes from this PR so it should be equivalent in behavior. - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2077319721
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v6]
On Thu, 25 Apr 2024 13:22:01 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove /jre path check Still looks good. We should maybe change the synopsis (title) of the bug to something like "libjli GetApplicationHome cleanups"? - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2077289573
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 13:38:56 GMT, Viktor Klang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comma-separated
>
> src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java line 208:
>
>> 206: default String displayDescriptor() {
>> 207: int count = parameterCount();
>> 208: StringBuilder sb = new StringBuilder(24).append('(');
>
> 24 is chosen by fair dice-roll? :)
More or less: default capacity is 16, 24 is 1.5x that - a pretty typical growth
factor. It also happens to be enough for very common descriptors such as
`()Ljava/lang/Object;` while not too large to cause a regression on very small
(and also very common) descriptors such as `()V`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18945#discussion_r1579533391
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 Thanks for the updates. I like that we have consistency between cgv1 and cgv2 in the latest version in terms of hierarchical limit. What would be even better is to get consistency between CPU and memory lookup (if the restriction is enforced higher up the hierarchy). That is, it would be ideal to make `initialize_hierarchy()` controller specific. Meanwhile I've been working on [some refactoring](https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb) which builds on top of [JDK-8302744](https://bugs.openjdk.org/browse/JDK-8302744) so as to make the code a bit nicer once this integrates. Then, the idea would be to use scratch controllers (`CgroupCpuController` and `CgroupMemoryController`) to determine whether or not there is a limit and figure out the actual path on a per-controller specific way - (use `CgroupMemoryController->read_memory_limit_in_bytes(phys_mem)` and `CgroupUtil::processor_count(CgroupCpuController* cpu_ctrl, int host_cpus)` in the process). Does that make sense? A few other observations: - The common case is when the JVM runs in a container. Then, the cgroup path is `/` on cgv2 and the and `root_mount == cgroup_path` on cgv1. We don't need to do the extra processing on those systems as the limit will be at the leaf. - The (fairly) uncommon case is the host case where the cgroup limit is applied elsewhere (e.g. systemd slice). This is where we'd need the hierarchy walk. - When we need to walk the hierarchy, we start at the longest path and only traverse if there is _NO_ limit. A system which sets a higher, limit (that isn't `max`), seems ill-defined and I've not come across one. As soon as we've found a lower value than unlimited (`-1`), we stop. Since cg2 is hierarchical, the lowest limit will affect the entire tree (corollary: higher values further down from that point won't have an effect): ``` /a/b --> memory.max 300 `- /c --> memory.max max (this wouldn't have any effect, therefore can be ignored). ``` - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2077263573
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v15]
> **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 one additional commit
since the last revision:
Revert
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18639/files
- new: https://git.openjdk.org/jdk/pull/18639/files/59cbeb69..9166be30
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18639=14
- incr: https://webrevs.openjdk.org/?repo=jdk=18639=13-14
Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 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: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 13:34:50 GMT, Claes Redestad wrote:
>> When analyzing (startup) performance of the Classfile API I found this
>> opportunity to further improve `MethodTypeDescImpl::descriptorString`.
>>
>> Performance improves across the board in existing microbenchmarks:
>>
>> Name
>> (descString) Cnt Base ErrorTest Error Unit Change
>> MethodTypeDescFactories.descriptorString
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189
>> ns/op 1,68x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636
>> ns/op 2,11x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p =
>> 0,000*)
>> * = significant
>>
>>
>> The improvement is even more pronounced when running with `-Xint`, which is
>> relevant for reducing startup overheads of early ClassFile API use:
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> MethodTypeDescFactories.descriptorString
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ±
>> 101,466 ns/op 1,95x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ±
>> 41,892 ns/op 2,36x (p = 0,000*)
>> MethodTypeDescFactories.descriptorString
>> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op
>> 2,46x (p = 0,000*)
>> * = significant
>> ```
>>
>> I also applied similar approach to `MethodTypeDesc::displayDescriptor`:
>> while not performance sensitive I think these are so inter-related that it
>> makes sense to implement them in a similar fashion.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> comma-separated
src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java line 208:
> 206: default String displayDescriptor() {
> 207: int count = parameterCount();
> 208: StringBuilder sb = new StringBuilder(24).append('(');
24 is chosen by fair dice-roll? :)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18945#discussion_r1579491160
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 13:34:50 GMT, Claes Redestad wrote: >> When analyzing (startup) performance of the Classfile API I found this >> opportunity to further improve `MethodTypeDescImpl::descriptorString`. >> >> Performance improves across the board in existing microbenchmarks: >> >> Name >> (descString) Cnt Base ErrorTest Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 >> ns/op 1,68x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 >> ns/op 2,11x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = >> 0,000*) >> * = significant >> >> >> The improvement is even more pronounced when running with `-Xint`, which is >> relevant for reducing startup overheads of early ClassFile API use: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.descriptorString >> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± >> 101,466 ns/op 1,95x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± >> 41,892 ns/op 2,36x (p = 0,000*) >> MethodTypeDescFactories.descriptorString >> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op >> 2,46x (p = 0,000*) >> * = significant >> ``` >> >> I also applied similar approach to `MethodTypeDesc::displayDescriptor`: >> while not performance sensitive I think these are so inter-related that it >> makes sense to implement them in a similar fashion. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > comma-separated Do we have any research on the average length of method descriptor strings? I wonder if manual pre-allocation works better (iterating all descriptor strings, allocate the sum of their sizes plus 2 (for parentheses), as descriptor strings won't be re-calculated after initial allocation.) Especially in case of user code, as many user classes have very long package names that can easily make the string much longer than the 24-char default. - PR Comment: https://git.openjdk.org/jdk/pull/18945#issuecomment-2077206533
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
> When analyzing (startup) performance of the Classfile API I found this > opportunity to further improve `MethodTypeDescImpl::descriptorString`. > > Performance improves across the board in existing microbenchmarks: > > Name > (descString) Cnt Base ErrorTest Error Unit Change > MethodTypeDescFactories.descriptorString > (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 > ns/op 1,68x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 > ns/op 2,11x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = > 0,000*) > * = significant > > > The improvement is even more pronounced when running with `-Xint`, which is > relevant for reducing startup overheads of early ClassFile API use: > > Name > (descString) Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.descriptorString > (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± > 101,466 ns/op 1,95x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± > 41,892 ns/op 2,36x (p = 0,000*) > MethodTypeDescFactories.descriptorString > ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op > 2,46x (p = 0,000*) > * = significant > ``` > > I also applied similar approach to `MethodTypeDesc::displayDescriptor`: > while not performance sensitive I think these are so inter-related that it > makes sense to implement them in a similar fashion. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: comma-separated - Changes: - all: https://git.openjdk.org/jdk/pull/18945/files - new: https://git.openjdk.org/jdk/pull/18945/files/06947305..2979847c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18945=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18945=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18945.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18945/head:pull/18945 PR: https://git.openjdk.org/jdk/pull/18945
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString [v2]
On Thu, 25 Apr 2024 11:11:48 GMT, Florent Guillaume wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comma-separated
>
> src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java line 210:
>
>> 208: StringBuilder sb = new StringBuilder(24).append('(');
>> 209: for (int i = 0; i < count; i++) {
>> 210: sb.append(parameterType(i).displayName());
>
> Aren't you forgetting the comma?
Well spotted - fixed!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18945#discussion_r1579476846
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v6]
> We have already good JLI tracing capabilities. But GetApplicationHome and > GetApplicationHomeFromDll lack some tracing and should be enhanced. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: remove /jre path check - Changes: - all: https://git.openjdk.org/jdk/pull/18699/files - new: https://git.openjdk.org/jdk/pull/18699/files/4d998244..4d8b6802 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18699=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18699=04-05 Stats: 24 lines in 2 files changed: 0 ins; 24 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18699.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18699/head:pull/18699 PR: https://git.openjdk.org/jdk/pull/18699
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 removed the mentioned 'private JRE' check and also the related size check. - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2077160938
Re: RFR: 8329760: Add indexOf(Predicate filter) to java.util.List interface [v14]
> **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 one additional commit
since the last revision:
Empty commit to rerun checks
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18639/files
- new: https://git.openjdk.org/jdk/pull/18639/files/d2f358b3..59cbeb69
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18639=13
- incr: https://webrevs.openjdk.org/?repo=jdk=18639=12-13
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Tue, 23 Apr 2024 14:29:05 GMT, Matthias Baesken wrote: > > `/* Does the app ship a private JRE in /jre directory? */` > > part meant? This looks indeed obsolete . Yes, this is code that doesn't make sense since JDK 9 and should be removed/cleanup at some point. I suspect we had to leave it at the time because of the issue of installers copying java.exe into a shared location and expecting it to work with any JRE or JDK release, something that doesn't make sense now of course. - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2077120542
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 LGTM - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18699#pullrequestreview-2022399268
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v2]
On Wed, 24 Apr 2024 15:27:23 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`. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Renamed package jdk.random to jdk.internal.random. I wish I could remember, or find the JBS issue or PR with the discussion, to remind us as to why the implementations were put into a separate module in the first place. The changes in the PR looks okay. I'm just wondering if we also need to re-examine the wording in RandomGeneratorFactory methods where it documents "uses the service provider API". I'm surprised this is documented as an implementation requirement. Also "service provider API" should really be ServiceLoader API. One other thing is that we'll need a release note for this change. It's possible there are scripts somewhere in the wild that use jlink and specify the jdk.random module in the sets of modules to include. These scrips will break with this change. I don't think it's worth leaving a hollowed out module in its place but we should at least document that it has been removed. - PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2077043094
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 Hi Lutz and Martin, thanks for the reviews! Jaikiran, are you done with your additional tests ? - PR Comment: https://git.openjdk.org/jdk/pull/18908#issuecomment-2077026022
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 19 Mar 2024 19:32:10 GMT, Joe Darcy wrote: >> I think splitting `CharArraySequence` into two versions is somewhat dubious >> as more observable types at call sites may mean the performance gain in >> targeted micros is lost. How much of an improvement did you observe from >> this? Again the `char[]` constructors is probably less performance sensitive >> than the others. > >> @cl4es @jddarcy All feedback has been fixed, can it be integrated? > > Hello @wenshao , > > This change will need additional review from myself or others who maintain > BigDecimal before it can be integrated. @jddarcy Sorry for the pings, Can you review this PR for me? - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2076967334
Re: RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString
On Thu, 25 Apr 2024 09:41:11 GMT, Claes Redestad wrote:
> When analyzing (startup) performance of the Classfile API I found this
> opportunity to further improve `MethodTypeDescImpl::descriptorString`.
>
> Performance improves across the board in existing microbenchmarks:
>
> Name
> (descString) Cnt Base ErrorTest Error Unit Change
> MethodTypeDescFactories.descriptorString
> (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189
> ns/op 1,68x (p = 0,000*)
> MethodTypeDescFactories.descriptorString
> ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*)
> MethodTypeDescFactories.descriptorString
> ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636
> ns/op 2,11x (p = 0,000*)
> MethodTypeDescFactories.descriptorString
> ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p =
> 0,000*)
> * = significant
>
>
> The improvement is even more pronounced when running with `-Xint`, which is
> relevant for reducing startup overheads of early ClassFile API use:
>
> Name
> (descString) Cnt Base Error Test Error Unit Change
> MethodTypeDescFactories.descriptorString
> (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ±
> 101,466 ns/op 1,95x (p = 0,000*)
> MethodTypeDescFactories.descriptorString
> ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*)
> MethodTypeDescFactories.descriptorString
> ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ±
> 41,892 ns/op 2,36x (p = 0,000*)
> MethodTypeDescFactories.descriptorString
> ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op
> 2,46x (p = 0,000*)
> * = significant
> ```
>
> I also applied similar approach to `MethodTypeDesc::displayDescriptor`:
> while not performance sensitive I think these are so inter-related that it
> makes sense to implement them in a similar fashion.
src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java line 210:
> 208: StringBuilder sb = new StringBuilder(24).append('(');
> 209: for (int i = 0; i < count; i++) {
> 210: sb.append(parameterType(i).displayName());
Aren't you forgetting the comma?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18945#discussion_r1579294651
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 Undefined behavior should always get fixed. Thanks for doing it! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18908#pullrequestreview-2022189272
RFR: 8331114: Further improve performance of MethodTypeDesc::descriptorString
When analyzing (startup) performance of the Classfile API I found this opportunity to further improve `MethodTypeDescImpl::descriptorString`. Performance improves across the board in existing microbenchmarks: Name (descString) Cnt Base ErrorTest Error Unit Change MethodTypeDescFactories.descriptorString (Ljava/lang/Object;Ljava/lang/String;)I 6 55,179 ± 2,027 32,920 ± 1,189 ns/op 1,68x (p = 0,000*) MethodTypeDescFactories.descriptorString ()V 6 17,689 ± 1,871 11,060 ± 0,331 ns/op 1,60x (p = 0,000*) MethodTypeDescFactories.descriptorString ([IJLjava/lang/String;Z)Ljava/util/List; 6 86,627 ± 1,646 41,035 ± 0,636 ns/op 2,11x (p = 0,000*) MethodTypeDescFactories.descriptorString ()[Ljava/lang/String; 6 18,305 ± 1,974 13,110 ± 0,089 ns/op 1,40x (p = 0,000*) * = significant The improvement is even more pronounced when running with `-Xint`, which is relevant for reducing startup overheads of early ClassFile API use: Name (descString) Cnt Base Error Test Error Unit Change MethodTypeDescFactories.descriptorString (Ljava/lang/Object;Ljava/lang/String;)I 6 5122,061 ± 81,335 2626,481 ± 101,466 ns/op 1,95x (p = 0,000*) MethodTypeDescFactories.descriptorString ()V 6 3481,316 ± 258,904 1489,267 ± 15,506 ns/op 2,34x (p = 0,000*) MethodTypeDescFactories.descriptorString ([IJLjava/lang/String;Z)Ljava/util/List; 6 7741,081 ± 1628,244 3281,778 ± 41,892 ns/op 2,36x (p = 0,000*) MethodTypeDescFactories.descriptorString ()[Ljava/lang/String; 6 3677,803 ± 63,432 1495,291 ± 8,995 ns/op 2,46x (p = 0,000*) * = significant ``` I also applied similar approach to `MethodTypeDesc::displayDescriptor`: while not performance sensitive I think these are so inter-related that it makes sense to implement them in a similar fashion. - Commit messages: - Optimize MethodTypeDescImpl::toString Changes: https://git.openjdk.org/jdk/pull/18945/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18945=00 Issue: https://bugs.openjdk.org/browse/JDK-8331114 Stats: 13 lines in 2 files changed: 3 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18945.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18945/head:pull/18945 PR: https://git.openjdk.org/jdk/pull/18945
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v25]
On Wed, 24 Apr 2024 23:15:45 GMT, Brent Christian wrote:
>> 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
src/java.base/share/classes/java/lang/ref/package-info.java line 153:
> 151: * The enqueueing of a reference (by the garbage collector, or
> 152: * by a successful call to {@link Reference#enqueue})
> happens-before
> 153: * the reference is removed from the queue (dequeued).
@bchristi-git I wonder if it makes sense to use the same style when referring
to "dequeue", so either always within quotes or only using em (see
line 108)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1579082626
Integrated: 8331097: Tests build is broken after pr/18914
On Thu, 25 Apr 2024 07:56:59 GMT, Adam Sotona wrote: > Pr/18914 broke tests build. > This patch fixes > `test/micro/org/openjdk/bench/jdk/classfile/CodeAttributeTools` > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: ef745a6c Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/ef745a6c6e4068e786a70fc5574d272140c01e0f Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8331097: Tests build is broken after pr/18914 Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/18943
Re: RFR: 8331097: Tests build is broken after pr/18914
On Thu, 25 Apr 2024 07:56:59 GMT, Adam Sotona wrote: > Pr/18914 broke tests build. > This patch fixes > `test/micro/org/openjdk/bench/jdk/classfile/CodeAttributeTools` > > Please review. > > Thanks, > Adam Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18943#pullrequestreview-2021821209
RFR: 8331097: Tests build is broken after pr/18914
Pr/18914 broke tests build. This patch fixes `test/micro/org/openjdk/bench/jdk/classfile/CodeAttributeTools` Please review. Thanks, Adam - Commit messages: - 8331097: Tests build is broken after pr/18914 Changes: https://git.openjdk.org/jdk/pull/18943/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18943=00 Issue: https://bugs.openjdk.org/browse/JDK-8331097 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18943.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18943/head:pull/18943 PR: https://git.openjdk.org/jdk/pull/18943
Integrated: 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 This pull request has now been integrated. Changeset: ccc0d0f7 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/ccc0d0f7b194a9941e2cadba1c389aa0834c52e4 Stats: 98 lines in 3 files changed: 56 ins; 31 del; 11 mod 8325373: Improve StackCounter error reporting for bad switch cases Reviewed-by: psandoz - PR: https://git.openjdk.org/jdk/pull/18914
Re: RFR: 8325373: Improve StackCounter error reporting for bad switch cases [v2]
> 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 Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/18914/files - new: https://git.openjdk.org/jdk/pull/18914/files/f59654be..1824d1fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18914=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18914=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18914.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18914/head:pull/18914 PR: https://git.openjdk.org/jdk/pull/18914
Re: RFR: 8325373: Improve StackCounter error reporting for bad switch cases [v2]
On Wed, 24 Apr 2024 22:48:30 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> applied suggested changes
>
> 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 `_`.
Both fixed, thanks for the review.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18914#discussion_r1578982248
Re: RFR: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
On Wed, 24 Apr 2024 22:43:21 GMT, Paul Sandoz wrote: > This looks good, but for completeness it will need a CSR. CSR created, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18913#issuecomment-2076498451
