Re: RFR: 8309821: Link to hidden classes section in Class specification for Class::isHidden
On Wed, 19 Jun 2024 02:21:05 GMT, Joe Darcy wrote: > Simple doc improvement. Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19781#pullrequestreview-2126919618
RFR: 8309821: Link to hidden classes section in Class specification for Class::isHidden
Simple doc improvement. - Commit messages: - JDK-8309821: Link to hidden classes section in Class specification for Class::isHidden Changes: https://git.openjdk.org/jdk/pull/19781/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19781=00 Issue: https://bugs.openjdk.org/browse/JDK-8309821 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19781.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19781/head:pull/19781 PR: https://git.openjdk.org/jdk/pull/19781
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Wed, 19 Jun 2024 02:12:01 GMT, lingjun-cg wrote:
>> src/java.base/share/classes/java/text/Format.java line 278:
>>
>>> 276: * {@code false} otherwise
>>> 277: */
>>> 278: boolean isInternalSubclass() {
>>
>> Since this is defined in Format, can we apply similar changes of
>> StringBuilder formatting to the other Format subclasses beyond just
>> NumberFormat.
>>
>> For example, in DateFormat, something such as,
>>
>>
>> T formatWithGeneric(Date date,
>>T toAppendTo,
>>FieldPosition pos) {
>> throw new UnsupportedOperationException("Subclasses should override
>> this method");
>> }
>
> ok. I will update it if we have a conclusion about using `StringBuf` or
> using ``.
This comment inspire me that use '' in
`SimpleDateFormat` is impossible, because the 'Appendable' lack of necessary
`append` method which SimpleDateFormat requires.
So we need discuss whether `StringBuf` is a better solution?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645309140
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 20:39:30 GMT, Justin Lu wrote:
>> lingjun-cg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 896: Performance regression of DecimalFormat.format
>
> src/java.base/share/classes/java/text/ChoiceFormat.java line 561:
>
>> 559: toAppendTo.append(choiceFormats[i]);
>> 560: } catch (IOException ioe) {
>> 561: throw new UncheckedIOException(ioe.getMessage(), ioe);
>
> Perhaps `AssertionError` instead of `UncheckedIOException` is better suited
> here and in the other ocurrences.
This can be removed if using `StringBuf`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645304136
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 20:36:52 GMT, Justin Lu wrote:
>> lingjun-cg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 896: Performance regression of DecimalFormat.format
>
> src/java.base/share/classes/java/text/Format.java line 278:
>
>> 276: * {@code false} otherwise
>> 277: */
>> 278: boolean isInternalSubclass() {
>
> Since this is defined in Format, can we apply similar changes of
> StringBuilder formatting to the other Format subclasses beyond just
> NumberFormat.
>
> For example, in DateFormat, something such as,
>
>
> T formatWithGeneric(Date date,
>T toAppendTo,
>FieldPosition pos) {
> throw new UnsupportedOperationException("Subclasses should override
> this method");
> }
ok. I will update it if we have a conclusion about using `StringBuf` or using
``.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645300958
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
Using ` ` instead of `StringBuf` seems a
good idea, but it has a disadvantage. The `Appendable` defines only 3 `append`
methods which cannot cover some cases. For example, this method
`SimpleDateFormat#format(Date, StringBuffer, FieldDelegate)` contains the
following code:
case TAG_QUOTE_CHARS:
toAppendTo.append(compiledPattern, i, count);
i += count;
break;
It requires `append(char[], int, int)`, but the `Appendable` has no such method.
So it's difficult to use ` ` in methods
`String DateFormat.format(Date date), String ListFormat.format(List
input) and, String MessageFormat.format(Object obj)`.
The method in `java.text.MessageFormat#subformat` contains the following code:
int argumentNumber = argumentNumbers[i];
if (arguments == null || argumentNumber >= arguments.length) {
result.append('{').append(argumentNumber).append('}');
continue;
}
It requires `append(int)`, but the `Appendable` has no such method.
To sum up
1. ` ` is clear, but lack of extensibility
2. StringBuf is not so clear, but it's more extensibility. We can add
`append(char[], int, int)` to `StringBuf` to support `SimpleDateFormat`.
3. New interface StringBuf no need to handle IOException, but use ` ` need to add a lot of try...catch statements around
the append method.
4.
So it seems `StringBuf` is better if we want to fix the problem in
`DateFormat,ListFormat,MessageFormat` in unique way.
I look forward to your reply. @naotoj @liach @justin-curtis-lu
Using ` ` instead of `StringBuf` seems a
good idea, but it has a disadvantage. The `Appendable` defines only 3 `append`
methods which cannot cover some cases. For example, this method
`SimpleDateFormat#format(Date, StringBuffer, FieldDelegate)` contains the
following code:
case TAG_QUOTE_CHARS:
toAppendTo.append(compiledPattern, i, count);
i += count;
break;
It requires `append(char[], int, int)`, but the `Appendable` has no such method.
So it's difficult to use ` ` in methods
`String DateFormat.format(Date date), String ListFormat.format(List
input) and, String MessageFormat.format(Object obj)`.
The method in `java.text.MessageFormat#subformat` contains the following code:
Re: RFR: 8333768: Minor doc updates to java.lang.{Float, Double} [v4]
On Fri, 7 Jun 2024 12:48:51 GMT, Raffaello Giulietti wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to review feedback. > > src/java.base/share/classes/java/lang/Double.java line 595: > >> 593: * This method corresponds to the convertToDecimalCharacter >> 594: * operation defined in IEEE 754. >> 595: * > > Does it? > > IEEE 754 `convertToDecimalCharacter` takes a 2nd argument, the > `conversionSpecification` which "specifies the precision and formatting of > the _decimalCharacterSequence_ result". There's no such flexibility here. > > Moreover, there seems to be no way to have a `conversionSpecification` that > ensures the "shortest decimal" semantics of this method. > > Finally, IEEE 754 requires to signal inexact exceptions. > > Suggestion: > > * This method vaguely corresponds to the convertToDecimalCharacter > > > The same holds for the other additional `@apiNote`s. > Does it? > > IEEE 754 `convertToDecimalCharacter` takes a 2nd argument, the > `conversionSpecification` which "specifies the precision and formatting of > the _decimalCharacterSequence_ result". There's no such flexibility here. > > Moreover, there seems to be no way to have a `conversionSpecification` that > ensures the "shortest decimal" semantics of this method. > > Finally, IEEE 754 requires to signal inexact exceptions. > > The same holds for the other additional `@apiNote`s. Thanks for the comments @rgiulietti . I've weakened the language for this method and added a javadoc to Formatter, which more closely aligns with the IEEE model of binary -> decimal conversion. For toHexString, the IEEE standard does state: "When converting to hexadecimal-significand character sequences in the absence of an explicit precision specification, enough hexadecimal characters shall be used to represent the binary floating-point number exactly." so the there isn't an exactly corresponding concern there. With the frame condition that the Java platform ignores the sticky flags, I think making the linkage to IEEE operations is still informative. Basically, (in many case) if you project down to the subset of IEEE 754 the Java platform supports, this Java method computes the same floating-point value as this IEEE operation, etc. - PR Review Comment: https://git.openjdk.org/jdk/pull/19590#discussion_r1645286967
Re: RFR: 8333768: Minor doc updates to java.lang.{Float, Double} [v4]
> Misc small doc updates and addition of `@Overrides` annotations. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.org/jdk/pull/19590/files - new: https://git.openjdk.org/jdk/pull/19590/files/9df534a3..4277c32b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19590=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19590=02-03 Stats: 30 lines in 2 files changed: 26 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19590.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19590/head:pull/19590 PR: https://git.openjdk.org/jdk/pull/19590
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v4]
On Tue, 18 Jun 2024 07:41:35 GMT, SendaoYan wrote: >> Hi all, >> Testcase >> `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` >> run fails with root user privileged. I think it's necessary to skip this >> testcase when user is root. >> Why run the jtreg test by root user? It's because during rpmbuild process >> for linux distribution of JDK, root user is the default user to build the >> openjdk, also is the default user to run the `make test-tier1`, this PR make >> this testcase more robustness. >> The change has been verified, only change the testcase, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id 8334333 to jtreg tag @bug Thanks all for the review. - PR Comment: https://git.openjdk.org/jdk/pull/19732#issuecomment-2177319690
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
> If not that, throw a StackOverflowException when run with
> DecimalFormat.format(double)
I'm not sure I understand this point.
I think this is on the right track and looks much better with the generic
implementation.
src/java.base/share/classes/java/text/ChoiceFormat.java line 561:
> 559: toAppendTo.append(choiceFormats[i]);
> 560: } catch (IOException ioe) {
> 561: throw new UncheckedIOException(ioe.getMessage(), ioe);
Perhaps `AssertionError` instead of `UncheckedIOException` is better suited
here and in the other ocurrences.
src/java.base/share/classes/java/text/Format.java line 278:
> 276: * {@code false} otherwise
> 277: */
> 278: boolean isInternalSubclass() {
Since this is defined in Format, can we apply similar changes of StringBuilder
formatting to the other Format subclasses beyond just NumberFormat.
For example, in DateFormat, something such as,
T formatWithGeneric(Date date,
T toAppendTo,
FieldPosition pos) {
throw new UnsupportedOperationException("Subclasses should override
this method");
}
-
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2126447188
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645029030
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645026453
Re: RFR: 8333768: Minor doc updates to java.lang.{Float, Double} [v3]
> Misc small doc updates and addition of `@Overrides` annotations.
Joe Darcy 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 three additional commits since the
last revision:
- Merge branch 'master' into JDK-8333768
- Merge branch 'master' into JDK-8333768
- JDK-8333768: Minor doc updates to java.lang.{Float, Double}
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19590/files
- new: https://git.openjdk.org/jdk/pull/19590/files/cfe77e62..9df534a3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19590=02
- incr: https://webrevs.openjdk.org/?repo=jdk=19590=01-02
Stats: 15989 lines in 652 files changed: 6385 ins; 7852 del; 1752 mod
Patch: https://git.openjdk.org/jdk/pull/19590.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19590/head:pull/19590
PR: https://git.openjdk.org/jdk/pull/19590
Re: RFR: 8334490: Normalize string with locale invariant `toLowerCase()`
On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato wrote: > The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for > string comparison, which should be avoided. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2126291246
Re: RFR: 8334490: Normalize string with locale invariant `toLowerCase()`
On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato wrote: > The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for > string comparison, which should be avoided. +1 - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2126251008
Re: RFR: 8334490: Normalize string with locale invariant `toLowerCase()`
On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato wrote: > The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for > string comparison, which should be avoided. LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2126244306
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v4]
On Tue, 18 Jun 2024 07:41:35 GMT, SendaoYan wrote: >> Hi all, >> Testcase >> `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` >> run fails with root user privileged. I think it's necessary to skip this >> testcase when user is root. >> Why run the jtreg test by root user? It's because during rpmbuild process >> for linux distribution of JDK, root user is the default user to build the >> openjdk, also is the default user to run the `make test-tier1`, this PR make >> this testcase more robustness. >> The change has been verified, only change the testcase, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id 8334333 to jtreg tag @bug Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19732#pullrequestreview-2126241928
RFR: 8334490: Normalize string with locale invariant `toLowerCase()`
The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for string comparison, which should be avoided. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19775/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19775=00 Issue: https://bugs.openjdk.org/browse/JDK-8334490 Stats: 7 lines in 1 file changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19775/head:pull/19775 PR: https://git.openjdk.org/jdk/pull/19775
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v4]
On Tue, 18 Jun 2024 07:41:35 GMT, SendaoYan wrote: >> Hi all, >> Testcase >> `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` >> run fails with root user privileged. I think it's necessary to skip this >> testcase when user is root. >> Why run the jtreg test by root user? It's because during rpmbuild process >> for linux distribution of JDK, root user is the default user to build the >> openjdk, also is the default user to run the `make test-tier1`, this PR make >> this testcase more robustness. >> The change has been verified, only change the testcase, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id 8334333 to jtreg tag @bug Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19732#pullrequestreview-2126195112
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:22:45 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge pull request #8 from cl4es/serialization_hostile > >SerializationHostileMethod > - Reduce gratuitous code movement > - Inline Consumer into generateSer.. method, move seldom-used > serialization support constants to new holder > - SerializationHostileMethod src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 509: > 507: } > 508: > 509: Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644860876
Re: RFR: 8333268: Fixes for static build [v2]
On Tue, 18 Jun 2024 16:19:39 GMT, Magnus Ihse Bursie wrote:
>> This patch contains a set of changes to improve static builds. They will
>> pave the way for implementing a full static-only java launcher. The changes
>> here will:
>>
>> 1) Make sure non-exported symbols are made local in the static libraries.
>> This means that the risk of symbol conflict is the same for static libraries
>> as for dynamic libraries (i.e. in practice zero, as long as a consistent
>> naming scheme is used for exported functions).
>>
>> 2) Remove the work-arounds to exclude duplicated symbols.
>>
>> 3) Fix some code in hotspot and the JDK libraries that did not work properly
>> with a static java launcher.
>>
>> The latter fixes are copied from or inspired by the work done by
>> @jianglizhou and her team as part of the Project Leyden [Hermetic
>> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).
>
> Magnus Ihse Bursie 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 seven additional
> commits since the last revision:
>
> - Merge branch 'master' into static-linking-progress
> - Merge branch 'master' into static-linking-progress
> - Move the exported JVM_IsStaticallyLinked to a better location
> - Use runtime lookup of static vs dynamic instead of #ifdef STATIC_BUILD
> - Copy fix for init_system_properties_values on linux
> - Make sure we do not try to build static libraries on Windows
> - 8333268: Fixes for static build
src/hotspot/os/linux/os_linux.cpp line 605:
> 603:
> 604: // Get rid of /{client|server|hotspot}, if binary is libjvm.so.
> 605: // Or, cut off /.
@jianglizhou This code is based on changes in the Hermetic Java repo, but I do
not fully understand neither the comment nor what the purpose is. If you could
explain this a bit I'd be grateful.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1644855137
Re: RFR: 8333268: Fixes for static build
On Thu, 30 May 2024 19:35:44 GMT, Magnus Ihse Bursie wrote: > Do os::lookup_function need to be implemented on Windows too, for symmetry, > even if it is only used on Unix platforms? @AlanBateman suggested to add `lookup_function` to os_windows.cpp as well, but just let it contain ShouldNotReachHere. That sounds like a good solution to me. - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2176657975
Re: RFR: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
On Mon, 17 Jun 2024 21:05:13 GMT, Justin Lu wrote: > Please review this PR, which is a trivial update of the IANA subtag registry > to version 2024-06-14. > Announcement: > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-June/92.html Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19757#pullrequestreview-2125989013
Re: RFR: 8333268: Fixes for static build [v2]
> This patch contains a set of changes to improve static builds. They will pave > the way for implementing a full static-only java launcher. The changes here > will: > > 1) Make sure non-exported symbols are made local in the static libraries. > This means that the risk of symbol conflict is the same for static libraries > as for dynamic libraries (i.e. in practice zero, as long as a consistent > naming scheme is used for exported functions). > > 2) Remove the work-arounds to exclude duplicated symbols. > > 3) Fix some code in hotspot and the JDK libraries that did not work properly > with a static java launcher. > > The latter fixes are copied from or inspired by the work done by @jianglizhou > and her team as part of the Project Leyden [Hermetic > Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). Magnus Ihse Bursie 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 seven additional commits since the last revision: - Merge branch 'master' into static-linking-progress - Merge branch 'master' into static-linking-progress - Move the exported JVM_IsStaticallyLinked to a better location - Use runtime lookup of static vs dynamic instead of #ifdef STATIC_BUILD - Copy fix for init_system_properties_values on linux - Make sure we do not try to build static libraries on Windows - 8333268: Fixes for static build - Changes: - all: https://git.openjdk.org/jdk/pull/19478/files - new: https://git.openjdk.org/jdk/pull/19478/files/6b24a789..e1c46562 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19478=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19478=00-01 Stats: 2608 lines in 114 files changed: 1321 ins; 955 del; 332 mod Patch: https://git.openjdk.org/jdk/pull/19478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19478/head:pull/19478 PR: https://git.openjdk.org/jdk/pull/19478
Re: RFR: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
On Mon, 17 Jun 2024 21:05:13 GMT, Justin Lu wrote: > Please review this PR, which is a trivial update of the IANA subtag registry > to version 2024-06-14. > Announcement: > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-June/92.html Marked as reviewed by srl (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19757#pullrequestreview-2125966725
Re: RFR: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
On Mon, 17 Jun 2024 21:05:13 GMT, Justin Lu wrote: > Please review this PR, which is a trivial update of the IANA subtag registry > to version 2024-06-14. > Announcement: > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-June/92.html Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19757#pullrequestreview-2125932353
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v22]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Inlined condy construction directly into CP entries - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/d3367487..857b8821 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=21 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=20-21 Stats: 9 lines in 1 file changed: 2 ins; 6 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v5]
On Thu, 13 Jun 2024 14:11:48 GMT, Chen Liang wrote: >> Please review this patch that fixes a critical issue that breaks some Proxy >> usages. >> >> Since the problematic patch from before cannot be backed out, this patch >> aims to emulate the old behavior before. A diff between before the >> problematic patch and this patch is available at >> https://gist.github.com/7565b2091008f561eb0ada019bc5e517, generated by >> running `git diff 326dbb1b139dd1ec1b8605339b91697cdf49da9a -- >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java`. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > methodField can be initialized eagerly Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/19615#issuecomment-2176158264
Integrated: 8333854: IllegalAccessError with proxies after JDK-8332457
On Mon, 10 Jun 2024 01:32:09 GMT, Chen Liang wrote: > Please review this patch that fixes a critical issue that breaks some Proxy > usages. > > Since the problematic patch from before cannot be backed out, this patch aims > to emulate the old behavior before. A diff between before the problematic > patch and this patch is available at > https://gist.github.com/7565b2091008f561eb0ada019bc5e517, generated by > running `git diff 326dbb1b139dd1ec1b8605339b91697cdf49da9a -- > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java`. This pull request has now been integrated. Changeset: 91bd85d6 Author:Chen Liang URL: https://git.openjdk.org/jdk/commit/91bd85d65dff9cea91b88da7ef241be5c7b85f94 Stats: 238 lines in 2 files changed: 161 ins; 8 del; 69 mod 8333854: IllegalAccessError with proxies after JDK-8332457 Reviewed-by: redestad, asotona - PR: https://git.openjdk.org/jdk/pull/19615
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:30:39 GMT, Claes Redestad wrote: >> Adam Sotona has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - Merge pull request #8 from cl4es/serialization_hostile >> >>SerializationHostileMethod >> - Reduce gratuitous code movement >> - Inline Consumer into generateSer.. method, move >> seldom-used serialization support constants to new holder >> - SerializationHostileMethod > > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > line 108: > >> 106: >> 107: // condy to load implMethod from class data >> 108: implMethodCondy = DynamicConstantDesc.ofNamed(BSM_CLASS_DATA, >> DEFAULT_NAME, CD_MethodHandle); > > Pre-existing tiny wart, but this one seem to be used only exceptionally (see > the comment/code around line 183) so it's probably better to inline the code > at the usage site rather than have a constant. If we aren't caching this descriptor, maybe it's even more simple for us to just construct the ConstantDynamicEntry from the pool builder at the use site; we can extract a method to keep this condy visible. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644501219
Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v5]
On Thu, 13 Jun 2024 14:11:48 GMT, Chen Liang wrote: >> Please review this patch that fixes a critical issue that breaks some Proxy >> usages. >> >> Since the problematic patch from before cannot be backed out, this patch >> aims to emulate the old behavior before. A diff between before the >> problematic patch and this patch is available at >> https://gist.github.com/7565b2091008f561eb0ada019bc5e517, generated by >> running `git diff 326dbb1b139dd1ec1b8605339b91697cdf49da9a -- >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java`. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > methodField can be initialized eagerly Marked as reviewed by redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19615#pullrequestreview-2125576655
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:22:45 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge pull request #8 from cl4es/serialization_hostile > >SerializationHostileMethod > - Reduce gratuitous code movement > - Inline Consumer into generateSer.. method, move seldom-used > serialization support constants to new holder > - SerializationHostileMethod I've done a final pass over this. Code changes overall look good - great even - and the only caveat is that there's going to be some added startup overhead in some apps from integrating this. A minimal app with a lambda expression might see a 2-3ms hit, for example. While there are ideas on how to trim down such overheads further I think this is a good time to draw a line in the sand and get this change integrated and exposed to a larger set of testing. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17108#pullrequestreview-2125569570
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
On Tue, 18 Jun 2024 13:22:45 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - Merge pull request #8 from cl4es/serialization_hostile > >SerializationHostileMethod > - Reduce gratuitous code movement > - Inline Consumer into generateSer.. method, move seldom-used > serialization support constants to new holder > - SerializationHostileMethod src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 108: > 106: > 107: // condy to load implMethod from class data > 108: implMethodCondy = DynamicConstantDesc.ofNamed(BSM_CLASS_DATA, > DEFAULT_NAME, CD_MethodHandle); Pre-existing tiny wart, but this one seem to be used only exceptionally (see the comment/code around line 183) so it's probably better to inline the code at the usage site rather than have a constant. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644473614
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v21]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with four additional commits since the last revision: - Merge pull request #8 from cl4es/serialization_hostile SerializationHostileMethod - Reduce gratuitous code movement - Inline Consumer into generateSer.. method, move seldom-used serialization support constants to new holder - SerializationHostileMethod - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/3aaf246e..d3367487 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=20 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=19-20 Stats: 86 lines in 1 file changed: 43 ins; 38 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Tue, 18 Jun 2024 11:33:51 GMT, Daniel Fuchs wrote: >> Agree with Kevin. To minimize risk, especially since this is to fix a >> significant regression and we are in RDP1, we are really trying to preserve >> the existing code as much as possible, even though it could be improved. > > It is also more error prone (which is why I suggested using a single well > tested utility method to implement the temporary hackery rather than > spreading it at different places in different forms). But I'm OK with the > code in this form. Thanks Daniel! - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644374757
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v19]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Additional test runs with SM enabled - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/384a3a19..697fc0d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19624=18 - incr: https://webrevs.openjdk.org/?repo=jdk=19624=17-18 Stats: 7 lines in 3 files changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
On Mon, 17 Jun 2024 20:51:34 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > braces Agreed that this is niche and will be short-lived, but I have added some SM-enabled runs with all permission policy for ThreadPoolAccTest.java and StartStopTest.java. These look trivial to add, and will be trivial to remove... Looking forward to more cleanup when we remove the SM-enabled paths. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2175956243
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 20:27:05 GMT, Sean Mullan wrote: >> Hi, >> We almost always check >> !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the >> RMIConnectionImpl contstructor) >> >> This keeps the "modern" case first (except in that constructor, where it is >> only one line for each side of the if...). >> >> This extra checking of >> SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the >> modern and old style cases distinct, based on other comments here. It keeps >> it simple to read I hope, but yes it is a little more verbose than it could >> be. >> >> I'm hoping you'll agree that as these checks will be removed anyway, >> probably with a release, we should delay more extensive cleanup until then. >> (I've also rolled back my noPermissionsACC removal to keep this change away >> from being a general cleanup.) >> >> I had a bunch of additional Exception handling for e.g. >> PrivilegedActionException I think in here, and it wasn't very pretty. But, >> it might look better when there are fewer nested ifs in these methods, and >> when we are properly in the post-SecurityManager world... 8-) >> >> Whether it checks acc != null or acc == null is based on the existing code. >> I think that makes this diff easier to read, but also could be reworked and >> be made more consistent after SM removal. > > Agree with Kevin. To minimize risk, especially since this is to fix a > significant regression and we are in RDP1, we are really trying to preserve > the existing code as much as possible, even though it could be improved. It is also more error prone (which is why I suggested using a single well tested utility method to implement the temporary hackery rather than spreading it at different places in different forms). But I'm OK with the code in this form. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644303322
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
On Mon, 17 Jun 2024 20:51:34 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> braces
I skimmed through the changes, much more readable now compared to some of the
initial revisions. I kinda agree with one of the comments from a few days ago
that if (allow) { .. } else { ..} is easier to read than putting the disallow
case first. It's not a big deal as this code will be changed very soon to
remove the SM path.
As regards testing then I think the highest priority should be to default/no-SM
case. We need to be confident that there are no regressions in JMX for this
execution mode. We don't have of course want to regression for the SM case but
that is a niche execution mode and not long for this world.
-
PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2175831168
Re: RFR: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
On Mon, 17 Jun 2024 21:05:13 GMT, Justin Lu wrote: > Please review this PR, which is a trivial update of the IANA subtag registry > to version 2024-06-14. > Announcement: > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-June/92.html Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19757#pullrequestreview-2125182766
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
On Mon, 17 Jun 2024 20:51:34 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> braces
Yes, maybe we are light on testing with an SM actually enabled.
AuthorizationTest is the key test here, and tests authenticated connections
with user/role names. That is passing with no SM, SM allowed, and SM enabled
with policy.
I am testing ThreadPoolAccTest.java with SM enabled with an allPermission
policy, as well as just SM allowed or not allowed. This is a good test as it
exercises the Monitor class. This still works, will add it.
Also, manual testing looks good to me:
In problem builds of jdk 23, attaching with JMX using authentication results in:
org.openjdk.kjdb.MyDebuggerException: getSubject is supported only if a
security manager is allowed
With this change, JMX attach using authentication works. A monitor role can
correctly get refusals like:
Caused by: java.lang.SecurityException: Access denied! Invalid access level for
requested MBeanServer operation.
at
com.sun.jmx.remote.security.MBeanServerFileAccessController.checkAccess(MBeanServerFileAccessController.java:348)
at
com.sun.jmx.remote.security.MBeanServerFileAccessController.checkWrite(MBeanServerFileAccessController.java:240)
at
com.sun.jmx.remote.security.MBeanServerAccessController.invoke(MBeanServerAccessController.java:469)
at
javax.management.remote.rmi.RMIConnectionImpl.doOperation(RMIConnectionImpl.java:1520)
...and a control role is accepted (that's JMX simple security at work).
Running the target with a SecurityManager, and attaching, I see e.g.:
org.openjdk.kjdb.MyDebuggerException: access denied
("javax.management.MBeanPermission"
"com.sun.management.internal.HotSpotThreadImpl#-[java.lang:type=Threading]"
"isInstanceOf")
...which looks correct.
Add a -Djava.security.policy=/my/all.policy
which has allPermission, and connections work OK. Removing AllPermission from
that policy, we get Exceptions again.
This looks good.
-
PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2175801724
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Mon, 17 Jun 2024 18:17:06 GMT, Justin Lu wrote:
>>> I second Justin's suggestion here. The change should benefit every
>>> implementation in the JDK, not only NumberFormat. Also, I might suggest
>>> renaming the new class, as it is kind of long/redundant. How about using
>>> something as simple as "StringBuf"?
>>
>> Thanks for your comment. The long name bother me for a while. I have changed
>> it to ""StringBuf".
>
> Hi @lingjun-cg
>
> Thanks for your work here.
>
> While this would benefit the performance of NumberFormat subclasses, we are
> hoping that if we are going to make the internal switch to StringBuilder, we
> can also make similar changes to other Format subclasses as well where
> applicable. So, we would want `isInternalSubclass()` to be defined at the
> Format level. Some methods that I can immediately think of would be `String
> DateFormat.format(Date date)`, `String ListFormat.format(List input)`
> and, `String MessageFormat.format(Object obj)`.
>
> I believe @naotoj shares the same sentiment.
@justin-curtis-lu The method `isInternalSubclass` in `DecimalFormat` already
pulls up to `Format`.
@liach @naotoj Using `Appendable & CharSequence` instead of an interface with a
limited method is a great idea. Following this idea, I have updated the PR. But
there are some notes when implementing the idea.
1. Rename `format` to `formatWithGeneric` in DecimalFormat that accepts
StringBuilder. If not that, throw a StackOverflowException when run with
`DecimalFormat.format(double)`. If not rename the format, there are 2 methods
in DecimalFormat:
```
@Override
public StringBuffer format(double number, StringBuffer result,
FieldPosition fieldPosition) {
return format(number, result, fieldPosition);
}
@Override
T format(double number, T result,
FieldPosition fieldPosition)
{
}
Because the second parameter type `StringBuffer` of the 1st method is more
concrete than type `T` of the 2nd method, so the 1st method will call itself
recursively.
2. The `Appendable#append(java.lang.CharSequence)` method can throws
IOException, so there is some messy code handling IOException.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2175778275
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Mon, 17 Jun 2024 11:21:37 GMT, Adam Sotona wrote: >> LMF will BMH and `ClassSpecializer`, but does not call into this code >> normally as the simple bound method handle needed by LMF is statically >> defined (`BoundMethodHandle.Species_L`). To be perfectly safe - and to keep >> lambda/indy/condy bootstrap overheads to a minumum - I'll continue >> recommending the desugaring of lambdas in java.lang.invoke > > It results in not very nice code, but makes sense. I'll note that replacing a lambda with an anonymous class has a very marginal effect on startup when amortizing the cost of setting up the infrastructure for spinning classes. What can have a decent effect is if we can replace a few lambdas/anon classes with more imperative code that spins up/loads fewer classes, e.g., desugaring streams to for loops. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1644202229
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
> ### Performance regression of DecimalFormat.format
> From the output of perf, we can see the hottest regions contain atomic
> instructions. But when run with JDK 11, there is no such problem. The reason
> is the removed biased locking.
> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
> contains many synchronized methods.
> So I added support for some new methods that accept StringBuilder which is
> lock-free.
>
> ### Benchmark testcase
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> private DecimalFormat format;
>
> @Setup(Level.Trial)
> public void setup() {
> format = new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testNewAndFormat() throws InterruptedException {
> new DecimalFormat("#0.0").format(9524234.1236457);
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testFormatOnly() throws InterruptedException {
> format.format(9524234.1236457);
> }
> }
>
>
> ### Test result
> Current JDK before optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> Current JDK after optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> ### JDK 11
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
lingjun-cg has updated the pull request incrementally with one additional
commit since the last revision:
896: Performance regression of DecimalFormat.format
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19513/files
- new: https://git.openjdk.org/jdk/pull/19513/files/e95f0928..1fde6a60
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19513=07
- incr: https://webrevs.openjdk.org/?repo=jdk=19513=06-07
Stats: 186 lines in 8 files changed: 130 ins; 0 del; 56 mod
Patch: https://git.openjdk.org/jdk/pull/19513.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19513/head:pull/19513
PR: https://git.openjdk.org/jdk/pull/19513
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps [v3]
> Extracting duplicate method references to static field reduce proxy class > spinning and loading. In this case 2 less classes loaded when using > `findAny()` on each type of stream. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fixes - Changes: - all: https://git.openjdk.org/jdk/pull/19477/files - new: https://git.openjdk.org/jdk/pull/19477/files/a223e608..d83a1c96 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19477=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19477=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19477.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19477/head:pull/19477 PR: https://git.openjdk.org/jdk/pull/19477
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps [v2]
> Extracting duplicate method references to static field reduce proxy class > spinning and loading. In this case 2 less classes loaded when using > `findAny()` on each type of stream. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: @ExE-Boss review - Changes: - all: https://git.openjdk.org/jdk/pull/19477/files - new: https://git.openjdk.org/jdk/pull/19477/files/cff6d034..a223e608 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19477=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19477=00-01 Stats: 40 lines in 1 file changed: 11 ins; 0 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/19477.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19477/head:pull/19477 PR: https://git.openjdk.org/jdk/pull/19477
RFR: 8334437: De-duplicate ProxyMethod list creation
Simple refactoring to extract identical, simple lambda expressions. Improve code clarity and reduce classes generated. - Commit messages: - Simplify - Refactor ProxyMethod list creation Changes: https://git.openjdk.org/jdk/pull/19760/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19760=00 Issue: https://bugs.openjdk.org/browse/JDK-8334437 Stats: 10 lines in 1 file changed: 4 ins; 4 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19760.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19760/head:pull/19760 PR: https://git.openjdk.org/jdk/pull/19760
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v3]
On Tue, 18 Jun 2024 07:31:59 GMT, Justin Lu wrote: >> SendaoYan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add a whitespace before if > > test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java > line 54: > >> 52: import jdk.test.lib.Utils; >> 53: import jdk.test.lib.process.ProcessTools; >> 54: import jdk.test.lib.Platform; > > It would be beneficial to add this issue's bug ID to the Jtreg `@bug` tag. Thanks for the suggestion. The bug id `8334333` has been added. - PR Review Comment: https://git.openjdk.org/jdk/pull/19732#discussion_r1643982602
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v4]
> Hi all, > Testcase > `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` > run fails with root user privileged. I think it's necessary to skip this > testcase when user is root. > Why run the jtreg test by root user? It's because during rpmbuild process for > linux distribution of JDK, root user is the default user to build the > openjdk, also is the default user to run the `make test-tier1`, this PR make > this testcase more robustness. > The change has been verified, only change the testcase, no risk. SendaoYan has updated the pull request incrementally with one additional commit since the last revision: add bug id 8334333 to jtreg tag @bug - Changes: - all: https://git.openjdk.org/jdk/pull/19732/files - new: https://git.openjdk.org/jdk/pull/19732/files/90d3b335..23f99429 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19732=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19732=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19732.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19732/head:pull/19732 PR: https://git.openjdk.org/jdk/pull/19732
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v3]
On Tue, 18 Jun 2024 06:31:37 GMT, SendaoYan wrote: >> Hi all, >> Testcase >> `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` >> run fails with root user privileged. I think it's necessary to skip this >> testcase when user is root. >> Why run the jtreg test by root user? It's because during rpmbuild process >> for linux distribution of JDK, root user is the default user to build the >> openjdk, also is the default user to run the `make test-tier1`, this PR make >> this testcase more robustness. >> The change has been verified, only change the testcase, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > add a whitespace before if test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java line 54: > 52: import jdk.test.lib.Utils; > 53: import jdk.test.lib.process.ProcessTools; > 54: import jdk.test.lib.Platform; It would be beneficial to add this issue's bug ID to the Jtreg `@bug` tag. - PR Review Comment: https://git.openjdk.org/jdk/pull/19732#discussion_r1643972916
Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v5]
On Thu, 13 Jun 2024 14:11:48 GMT, Chen Liang wrote: >> Please review this patch that fixes a critical issue that breaks some Proxy >> usages. >> >> Since the problematic patch from before cannot be backed out, this patch >> aims to emulate the old behavior before. A diff between before the >> problematic patch and this patch is available at >> https://gist.github.com/7565b2091008f561eb0ada019bc5e517, generated by >> running `git diff 326dbb1b139dd1ec1b8605339b91697cdf49da9a -- >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java`. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > methodField can be initialized eagerly It is unfortunate that LDC of a CP entry cannot substitute the original complex approach (and the critical cases were not covered by any tests). @liach great work on the restoration of the original approach, while keeping the other improvements Thanks! - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19615#pullrequestreview-2124720977
Integrated: 8331431: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Tested: tier 1 — tier 8 This pull request has now been integrated. Changeset: 99fefec0 Author:Christian Stein URL: https://git.openjdk.org/jdk/commit/99fefec092f49cd759f93aa75e008cfa06d2a183 Stats: 12 lines in 8 files changed: 0 ins; 0 del; 12 mod 8331431: Update to use jtreg 7.4 Reviewed-by: ihse, erikj, jpai - PR: https://git.openjdk.org/jdk/pull/19052
Re: RFR: 8331431: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Tested: tier 1 — tier 8 Tests in tier 6-8 also look good. About to make `jtreg 7.4` the default now. - PR Comment: https://git.openjdk.org/jdk/pull/19052#issuecomment-2175335129
Re: RFR: 8333867: SHA3 performance can be improved [v4]
> This PR removes some unnecessary conversions between byte arrays and long > arrays during SHA3 digest computations. Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision: Copyright year update. - Changes: - all: https://git.openjdk.org/jdk/pull/19632/files - new: https://git.openjdk.org/jdk/pull/19632/files/4fcdd09f..2bcd3000 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19632=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19632=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19632.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19632/head:pull/19632 PR: https://git.openjdk.org/jdk/pull/19632
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v3]
> Hi all, > Testcase > `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` > run fails with root user privileged. I think it's necessary to skip this > testcase when user is root. > Why run the jtreg test by root user? It's because during rpmbuild process for > linux distribution of JDK, root user is the default user to build the > openjdk, also is the default user to run the `make test-tier1`, this PR make > this testcase more robustness. > The change has been verified, only change the testcase, no risk. SendaoYan has updated the pull request incrementally with one additional commit since the last revision: add a whitespace before if - Changes: - all: https://git.openjdk.org/jdk/pull/19732/files - new: https://git.openjdk.org/jdk/pull/19732/files/9b8a0bcb..90d3b335 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19732=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19732=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19732.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19732/head:pull/19732 PR: https://git.openjdk.org/jdk/pull/19732
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v2]
On Tue, 18 Jun 2024 06:18:48 GMT, Andrey Turbanov wrote:
>> SendaoYan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> change the excption meassges to: Unable to create an unreadable properties
>> file
>
> test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java
> line 59:
>
>> 57: public class MissingResourceCauseTestRun {
>> 58: public static void main(String[] args) throws Throwable {
>> 59: if(Platform.isRoot() && !Platform.isWindows()) {
>
> Suggestion:
>
> if (Platform.isRoot() && !Platform.isWindows()) {
Thanks for the review. A white space before `if` has been added.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19732#discussion_r1643892712
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v2]
On Tue, 18 Jun 2024 02:00:41 GMT, SendaoYan wrote:
>> Hi all,
>> Testcase
>> `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java`
>> run fails with root user privileged. I think it's necessary to skip this
>> testcase when user is root.
>> Why run the jtreg test by root user? It's because during rpmbuild process
>> for linux distribution of JDK, root user is the default user to build the
>> openjdk, also is the default user to run the `make test-tier1`, this PR make
>> this testcase more robustness.
>> The change has been verified, only change the testcase, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change the excption meassges to: Unable to create an unreadable properties
> file
test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java line
59:
> 57: public class MissingResourceCauseTestRun {
> 58: public static void main(String[] args) throws Throwable {
> 59: if(Platform.isRoot() && !Platform.isWindows()) {
Suggestion:
if (Platform.isRoot() && !Platform.isWindows()) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19732#discussion_r1643875207
Withdrawn: 8333396: Performance regression of DecimalFormat.format
On Mon, 3 Jun 2024 04:18:20 GMT, lingjun-cg wrote:
> ### Performance regression of DecimalFormat.format
> From the output of perf, we can see the hottest regions contain atomic
> instructions. But when run with JDK 11, there is no such problem. The reason
> is the removed biased locking.
> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
> contains many synchronized methods.
> So I added support for some new methods that accept StringBuilder which is
> lock-free.
>
> ### Benchmark testcase
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> private DecimalFormat format;
>
> @Setup(Level.Trial)
> public void setup() {
> format = new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testNewAndFormat() throws InterruptedException {
> new DecimalFormat("#0.0").format(9524234.1236457);
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testFormatOnly() throws InterruptedException {
> format.format(9524234.1236457);
> }
> }
>
>
> ### Test result
> Current JDK before optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> Current JDK after optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> ### JDK 11
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/19513
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v7]
> ### Performance regression of DecimalFormat.format
> From the output of perf, we can see the hottest regions contain atomic
> instructions. But when run with JDK 11, there is no such problem. The reason
> is the removed biased locking.
> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
> contains many synchronized methods.
> So I added support for some new methods that accept StringBuilder which is
> lock-free.
>
> ### Benchmark testcase
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> private DecimalFormat format;
>
> @Setup(Level.Trial)
> public void setup() {
> format = new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testNewAndFormat() throws InterruptedException {
> new DecimalFormat("#0.0").format(9524234.1236457);
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testFormatOnly() throws InterruptedException {
> format.format(9524234.1236457);
> }
> }
>
>
> ### Test result
> Current JDK before optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> Current JDK after optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> ### JDK 11
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
lingjun-cg has updated the pull request with a new target base due to a merge
or a rebase.
-
Changes: https://git.openjdk.org/jdk/pull/19513/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19513=06
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19513.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19513/head:pull/19513
PR: https://git.openjdk.org/jdk/pull/19513
