Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Tue, 25 Jun 2024 13:59:35 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [JDK-808](https://bugs.openjdk.org/browse/JDK-808) > enabling `javap -system` to handle internal class names. > > Thanks, > Sonia +1 - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19883#pullrequestreview-2144277865
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Wed, 26 Jun 2024 09:43:32 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
Thanks for all the updates. Looks good to me pending the other's comments.
(BTW, the benchmark can go to -> _test/micro/org/openjdk/bench/java/text/_ as
opposed to _test/jdk/java/text_)
src/java.base/share/classes/java/text/CompactNumberFormat.java line 569:
> 567: @Override
> 568: StringBuilder format(Object number,
> 569: StringBuilder toAppendTo,
nit: indentation seems odd here/not consistent with the other corresponding
method.
src/java.base/share/classes/java/text/SimpleDateFormat.java line 1330:
> 1328:
> 1329: int num = (value / 60) * 100 + (value % 60);
> 1330: if (buffer.isProxyStringBuilder()) {
If we made the implementations of `StringBuf` records, we could use record
patterns and do away with the `isProxyStringBuilder()` method. Although we
would have to change the visibility of the implementations, so maybe not...
src/java.base/share/classes/java/text/StringBufFactory.java line 29:
> 27:
> 28: /**
> 29: * StringBufFactory create {code Format.StringBuf}'s implements that
Nit: Just to improve some grammar, how about something like,
* {@code StringBufFactory} creates implementations of {@code Format.StringBuf},
* which is an interface with the minimum overlap required to support {@code
StringBuffer}
* and {@code StringBuilder} in {@code Format}. This allows for {@code
StringBuilder} to be used
* in place of {@code StringBuffer} to provide performance benefits for JDK
internal
* {@code Format} subclasses.
src/java.base/share/classes/java/text/StringBufFactory.java line 37:
> 35: final class StringBufFactory {
> 36:
> 37: static Format.StringBuf of(StringBuffer sb) {
At least for the factory class, let's just `import
java.text.Format.StringBuf;`, so we don't have to use the full name everywhere.
src/java.base/share/classes/java/text/StringBufFactory.java line 45:
> 43: }
> 44:
> 45: private static class StringBufferImpl implements Format.StringBuf {
The implementations may be more concise as a `record class`
-
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2144190566
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656424171
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656446980
PR Review Comment:
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v20]
On Wed, 26 Jun 2024 14:55:44 GMT, Shaojin Wen wrote:
>> The current versions of FloatToDecimal and DoubleToDecimal allocate
>> additional objects. Reducing these allocations can improve the performance
>> of Float/Double.toString and AbstractStringBuilder's append(float/double).
>>
>> This patch is just a code refactoring to reduce object allocation, but does
>> not change the Float/Double to decimal algorithm.
>>
>> The following code comments the allocated objects to be removed.
>>
>>
>> class FloatToDecimal {
>> public static String toString(float v) {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(float v, Appendable app)
>> throws IOException {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(float v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> if (app instanceof StringBuilder builder) {
>> return builder.append(chars);
>> }
>> if (app instanceof StringBuffer buffer) {
>> return buffer.append(chars);
>> }
>> for (char c : chars) {
>> app.append(c);
>> }
>> return app;
>> // ...
>> }
>> }
>> }
>>
>> class DoubleToDecimal {
>> public static String toString(double v) {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(double v, Appendable app)
>> throws IOException {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(double v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v, null)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> ...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. revert code change.
> 2. comment remove space
Tier 1-3 tests look good. Thanks Wen Shaojin!
-
PR Comment: https://git.openjdk.org/jdk/pull/19730#issuecomment-2193714370
Integrated: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal
On Sat, 15 Jun 2024 01:59:42 GMT, Shaojin Wen wrote:
> The current versions of FloatToDecimal and DoubleToDecimal allocate
> additional objects. Reducing these allocations can improve the performance of
> Float/Double.toString and AbstractStringBuilder's append(float/double).
>
> This patch is just a code refactoring to reduce object allocation, but does
> not change the Float/Double to decimal algorithm.
>
> The following code comments the allocated objects to be removed.
>
>
> class FloatToDecimal {
> public static String toString(float v) {
> // allocate object FloatToDecimal
> return new FloatToDecimal().toDecimalString(v);
> }
>
> public static Appendable appendTo(float v, Appendable app)
> throws IOException {
> // allocate object FloatToDecimal
> return new FloatToDecimal().appendDecimalTo(v, app);
> }
>
> private Appendable appendDecimalTo(float v, Appendable app)
> throws IOException {
> switch (toDecimal(v)) {
> case NON_SPECIAL:
> // allocate object char[]
> char[] chars = new char[index + 1];
> for (int i = 0; i < chars.length; ++i) {
> chars[i] = (char) bytes[i];
> }
> if (app instanceof StringBuilder builder) {
> return builder.append(chars);
> }
> if (app instanceof StringBuffer buffer) {
> return buffer.append(chars);
> }
> for (char c : chars) {
> app.append(c);
> }
> return app;
> // ...
> }
> }
> }
>
> class DoubleToDecimal {
> public static String toString(double v) {
> // allocate object DoubleToDecimal
> return new DoubleToDecimal(false).toDecimalString(v);
> }
>
> public static Appendable appendTo(double v, Appendable app)
> throws IOException {
> // allocate object DoubleToDecimal
> return new DoubleToDecimal(false).appendDecimalTo(v, app);
> }
>
> private Appendable appendDecimalTo(double v, Appendable app)
> throws IOException {
> switch (toDecimal(v, null)) {
> case NON_SPECIAL:
> // allocate object char[]
> char[] chars = new char[index + 1];
> for (int i = 0; i < chars.length; ++i) {
> chars[i] = (char) bytes[i];
> }
> if (app instanceof StringBuilder builder) {
> return builder.append(chars);
> }
> ...
This pull request has now been integrated.
Changeset: 9d20b58f
Author:Shaojin Wen
Committer: Chen Liang
URL:
https://git.openjdk.org/jdk/commit/9d20b58f40275002afa0348d94d5592a26894e88
Stats: 613 lines in 5 files changed: 264 ins; 212 del; 137 mod
8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal
Reviewed-by: redestad, rgiulietti
-
PR: https://git.openjdk.org/jdk/pull/19730
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Wed, 26 Jun 2024 09:43:32 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
src/java.base/share/classes/java/text/Format.java line 193:
> 191:
> 192: StringBuilder format(Object obj,
> 193: StringBuilder toAppendTo,
Can we change this type to `StringBuf`, like `StringBuf format(Object obj,
StringBuf toAppendTo, FieldPosition pos)`? Then we can remove a few
`StringBuilder` overloads by just using the `StringBuf` version.
src/java.base/share/classes/java/text/NumberFormat.java line 424:
> 422:
> 423: StringBuilder format(double number,
> 424: StringBuilder toAppendTo,
Same remark, `StringBuf format(double number, StringBuf toAppendTo,
FieldPosition pos)`
src/java.base/share/classes/java/text/SimpleDateFormat.java line 1034:
> 1032: @Override
> 1033: public AttributedCharacterIterator formatToCharacterIterator(Object
> obj) {
> 1034: StringBuilder sb = new StringBuilder();
On second thought, we can just make this `StringBuf sb =
StringBufFactory.of(new StringBuilder())` - or just add `StringBufFactory.of()`
which defaults to create a StringBuilder-backed buf.
src/java.base/share/classes/java/text/SimpleDateFormat.java line 1448:
> 1446: numberFormat.setMaximumIntegerDigits(maxDigits);
> 1447: if (buffer.isProxyStringBuilder()) {
> 1448: numberFormat.format((long)value, buffer.asStringBuilder(),
> DontCareFieldPosition.INSTANCE);
This `numberFormat` might be a user class; if that's the case, we might do
something like:
if (buffer.isProxyStringBuilder()) {
var nf = numberFormat; // field is protected, users can change it even with
races!
if (nf.isInternalSubclass()) {
nf.format((long)value, buffer.asStringBuilder(),
DontCareFieldPosition.INSTANCE);
} else {
// format to stringbuffer, and add that buffer to the builder
buffer.append(nf.format((long)value, new StringBuffer(),
DontCareFieldPosition.INSTANCE));
}
} else { // existing code
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655595282
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655596623
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655605616
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655603917
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Wed, 26 Jun 2024 09:43:32 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
Thanks for the changes. Since this is a performance improvement, I also would
like to have the microbenchmark that you used as the test case.
src/java.base/share/classes/java/text/Format.java line 427:
> 425: /**
> 426: * Used to distinguish JDK internal subclass and user-defined
> subclass
> 427: * of {code Format}.
There are a lot of places which is missing `@` for `code` tag in the comments.
src/java.base/share/classes/java/text/Format.java line 434:
> 432: boolean isInternalSubclass() {
> 433: return false;
> 434: }
Do we need this? Should comparing the package name with 'java.text' be
sufficient?
src/java.base/share/classes/java/text/Format.java line 438:
> 436: /**
> 437: * StringBuf is the minimal common interface of {code StringBuffer}
> and {code StringBuilder}.
> 438: * It used by the various {code Format} implementations as the
> internal string buffer.
typo: "is" is missing
src/java.base/share/classes/java/text/Format.java line 440:
> 438: * It used by the various {code Format} implementations as the
> internal string buffer.
> 439: */
> 440: interface StringBuf {
Can we make it `sealed`?
src/java.base/share/classes/java/text/ListFormat.java line 365:
> 363: return format(input, new StringBuffer(),
> 364: DontCareFieldPosition.INSTANCE).toString();
> 365: }
Since `ListFormat` is a final class, no need to have a condition, always use
StringBuilder version.
src/java.base/share/classes/java/text/StringBufFactory.java line 35:
> 33: * a better performance.
> 34: */
> 35: final class StringBufFactory {
Add a private no-arg constructor so that no instance can be created for
`StringBufFactory` itself.
-
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2143027077
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655560390
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655563089
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655565976
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r169020
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655566915
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655571771
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null [v13]
On Sun, 23 Jun 2024 08:47:25 GMT, Shaojin Wen wrote: >> After PR https://github.com/openjdk/jdk/pull/16245, C2 optimizes stores into >> primitive arrays by combining values into larger stores. >> >> This PR rewrites the code of appendNull and append(boolean) methods so that >> these two methods can be optimized by C2. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > private static final field `UNSAFE` Optimizing in C2 is a better approach and worth waiting for. - PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2192703305
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v20]
On Wed, 26 Jun 2024 17:42:13 GMT, Chen Liang wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 1. revert code change.
>> 2. comment remove space
>
> src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 194:
>
>> 192: * otherwise NON_SPECIAL
>> 193: */
>> 194: private int toDecimal(byte[] str, int index, double v,
>> FormattedFPDecimal fd) {
>
> This `toDecimal` returns packed int with info but the other returns the new
> index; this is extremely confusing for future readers, especially that they
> have very similar signatures (difference in signature is in the middle). I
> recommend making this return a `long` so when it's downcasted to an int, it's
> just the size (like String concat's coderIndex)
The maximum length of float to decimal is 15, and the maximum length of double
to decimal is 24, so one byte is enough. If we use the long return type, we
have to add `cast int to long` and `cast long to int` to the code, which is
redundant. It will also confuse people, thinking that float to decimal and
double to decimal will have a large length, just like the size of string
concat, which requires a complete int.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1655573216
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 18:08:21 GMT, Nizar Benalla wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - use javadoc standards
>> - Merge remote-tracking branch 'upstream/master' into naming
>> - remove whitespace
>> - 8332072: Convert package.html files in `java.naming` to package-info.java
>
> src/java.naming/share/classes/javax/naming/ldap/package-info.java line 200:
>
>> 198: * if (respCtls != null) {
>> 199: * // Find the one we want
>> 200: * for (int i = 0; i < respCtls.length; i++) {
>
> This is the only change that isn't a direct 1:1 conversion, I hope this ok.
This seems reasonable for what is an obvious but minor bug
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1655501908
Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Tue, 25 Jun 2024 13:59:35 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [JDK-808](https://bugs.openjdk.org/browse/JDK-808) > enabling `javap -system` to handle internal class names. > > Thanks, > Sonia noreg means no regression test. Since we cannot really create a shadow system modules to place alternative class files to render in javap, we probably just don't use a regression test. - Marked as reviewed by liach (Committer). PR Review: https://git.openjdk.org/jdk/pull/19883#pullrequestreview-2142826727
Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Tue, 25 Jun 2024 13:59:35 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [JDK-808](https://bugs.openjdk.org/browse/JDK-808) > enabling `javap -system` to handle internal class names. > > Thanks, > Sonia Hi everyone, thanks for taking a look. Seeing the challenges involved with creating a jtreg test, what would you recommend as a viable way forward with this patch? - PR Comment: https://git.openjdk.org/jdk/pull/19883#issuecomment-2192516991
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null [v4]
On Thu, 20 Jun 2024 14:32:25 GMT, Shaojin Wen wrote:
>> @cl4es @wenshao I think we should probably work on `putChar`, or at least
>> figure out what blocks `MergeStore` for `putChar`. Can someone produce a
>> simple stand-alone `.java` file that uses `putChar`, so that I can
>> investigate why `MergeStore` does not work for it?
>>
>> Otherwise, I agree with @cl4es : the code here may be ok for now, but we
>> would have to revert it again later, should `MergeStore` eventually do the
>> trick.
>
> @eme64
>
> simple stand-alone java
>
> import jdk.internal.misc.Unsafe;
>
> public class PutCharTest {
> static final Unsafe UNSAFE = Unsafe.getUnsafe();
>
> static void putCharsAt(byte[] val, int index, int c1, int c2, int c3, int
> c4) {
> putChar(val, index, (char)(c1));
> putChar(val, index + 1, (char)(c2));
> putChar(val, index + 2, (char)(c3));
> putChar(val, index + 3, (char)(c4));
> }
>
> static void putChar(byte[] bytes, int index, int c) {
> UNSAFE.putChar(bytes, Unsafe.ARRAY_BYTE_BASE_OFFSET + (index << 1),
> (char)(c));
> }
>
> static void putChar0(byte[] bytes, int index, int c) {
> UNSAFE.putByte(bytes, Unsafe.ARRAY_BYTE_BASE_OFFSET + (index << 1),
> (byte)(c));
> UNSAFE.putByte(bytes, Unsafe.ARRAY_BYTE_BASE_OFFSET + (index << 1) +
> 1, (byte)(c << 8));
> }
>
> static void putNull(byte[] bytes, int index) {
> putCharsAt(bytes, index, 'n', 'u', 'l', 'l');
> }
>
> public static void main(String[] args) {
> for (int i = 0; i < 5; i++) {
> testNull();
> }
>
> System.out.println("done");
> }
>
> private static void testNull() {
> byte[] bytes = new byte[8192];
>
> for (int i = 0; i < 1000; i++) {
> int index = 0;
> for (int j = 0; j < 1024; j++) {
> putNull(bytes, index);
> index += 4;
> }
> }
> }
> }
As long as @wenshao is OK with that I'm happy to let this wait. I'll have
limited availability myself in the coming weeks.
-
PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2192459124
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
On Wed, 26 Jun 2024 13:02:51 GMT, Jan Lahoda wrote:
>> test/micro/org/openjdk/bench/java/lang/runtime/SwitchEnum.java line 57:
>>
>>> 55: for (E e : inputs) {
>>> 56: sum += switch (e) {
>>> 57: case null -> -1;
>>
>> As this `null` case adds a case relative to the `-Traditional` test then
>> maybe removing one of the `E0, E1, ...` cases would make the test a little
>> bit more apples-to-apples?
>
> Using `case null -> ` will push javac to use the new code, but all switches
> do some kind of null check for the selector value. The difference is that if
> there's no `case null`, there will be `Objects.requireNonNull` generated for
> the selector value (which will throw an NPE if the value is `null`), while
> here it will jump to the given case.
>
> So, `case null` does not have the same weight as a normal case, so I don't
> think it would be fair to remove a full case to compensate for it.
Fair enough, and I guess ~1.6ns/op is reasonable overhead for the added
semantics.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1655364063
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v20]
On Wed, 26 Jun 2024 14:55:44 GMT, Shaojin Wen wrote:
>> The current versions of FloatToDecimal and DoubleToDecimal allocate
>> additional objects. Reducing these allocations can improve the performance
>> of Float/Double.toString and AbstractStringBuilder's append(float/double).
>>
>> This patch is just a code refactoring to reduce object allocation, but does
>> not change the Float/Double to decimal algorithm.
>>
>> The following code comments the allocated objects to be removed.
>>
>>
>> class FloatToDecimal {
>> public static String toString(float v) {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(float v, Appendable app)
>> throws IOException {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(float v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> if (app instanceof StringBuilder builder) {
>> return builder.append(chars);
>> }
>> if (app instanceof StringBuffer buffer) {
>> return buffer.append(chars);
>> }
>> for (char c : chars) {
>> app.append(c);
>> }
>> return app;
>> // ...
>> }
>> }
>> }
>>
>> class DoubleToDecimal {
>> public static String toString(double v) {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(double v, Appendable app)
>> throws IOException {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(double v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v, null)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> ...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. revert code change.
> 2. comment remove space
LGTM2
-
Marked as reviewed by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19730#pullrequestreview-2142672406
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
> Can I please get a review for this small change? The motivation is that javac > does not recognize `package.html` files. > > The conversion was simple, I used a script to rename the files, append "*" on > the left and remove some HTML tags like `` and ``. I did the > conversion in place, renaming them in git but with the big amount of change > `git` thinks it's a new file. > > I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I > hope that's fine. Nizar Benalla has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - use javadoc standards - Merge remote-tracking branch 'upstream/master' into naming - remove whitespace - 8332072: Convert package.html files in `java.naming` to package-info.java - Changes: - all: https://git.openjdk.org/jdk/pull/19529/files - new: https://git.openjdk.org/jdk/pull/19529/files/768eebf5..1826633f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19529=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19529=00-01 Stats: 67137 lines in 1456 files changed: 42388 ins; 18751 del; 5998 mod Patch: https://git.openjdk.org/jdk/pull/19529.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19529/head:pull/19529 PR: https://git.openjdk.org/jdk/pull/19529
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 18:09:10 GMT, Nizar Benalla wrote:
>> Can I please get a review for this small change? The motivation is that
>> javac does not recognize `package.html` files.
>>
>> The conversion was simple, I used a script to rename the files, append "*"
>> on the left and remove some HTML tags like `` and ``. I did the
>> conversion in place, renaming them in git but with the big amount of change
>> `git` thinks it's a new file.
>>
>> I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I
>> hope that's fine.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - use javadoc standards
> - Merge remote-tracking branch 'upstream/master' into naming
> - remove whitespace
> - 8332072: Convert package.html files in `java.naming` to package-info.java
I have converted this to use some of the now-current javadoc convetions.
src/java.naming/share/classes/javax/naming/ldap/package-info.java line 200:
> 198: * if (respCtls != null) {
> 199: * // Find the one we want
> 200: * for (int i = 0; i < respCtls.length; i++) {
This is the only change that isn't a direct 1:1 conversion, I hope this ok.
-
PR Review: https://git.openjdk.org/jdk/pull/19529#pullrequestreview-2142622498
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1655320955
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v6]
On Wed, 26 Jun 2024 17:59:54 GMT, Naoto Sato wrote: >> src/java.base/share/classes/java/text/ChoiceFormat.java line 591: >> >>> 589: /** >>> 590: * @since 23 >>> 591: */ >> >> IIUC, this addition will add & inherit the javadoc from `NumberFormat`, >> which is not the case before. The description for NumberFormat does not fit >> with ChoiceFormat. Probably that needs to be addressed with a different >> issue. > > My review is for the ChoiceFormat class only. So asking for more Reviews for > other area Thanks, I will leave this open for a few more days to let more people from the relevant areas review it. - PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1655315793
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v6]
On Wed, 26 Jun 2024 17:53:07 GMT, Naoto Sato wrote: >> Nizar Benalla has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add new line when having multi-line doc comment > > src/java.base/share/classes/java/text/ChoiceFormat.java line 591: > >> 589: /** >> 590: * @since 23 >> 591: */ > > IIUC, this addition will add & inherit the javadoc from `NumberFormat`, which > is not the case before. The description for NumberFormat does not fit with > ChoiceFormat. Probably that needs to be addressed with a different issue. My review is for the ChoiceFormat class only. So asking for more Reviews for other area - PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1655309499
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v6]
On Wed, 26 Jun 2024 17:07:27 GMT, Nizar Benalla wrote: >> Please review this PR that aims to add all the remaining needed `@since` >> tags in `java.base`, and group them into a single fix. >> This is related to #18934 and my work around the `@since` checker feature. >> Explicit `@since` tags are needed for some overriding methods for the >> purpose of the checker. >> >> I will only give the example with the `CompletableFuture` class but here is >> the before where the methods only appeared in "Methods declared in interface >> N" >> >> > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac;> >> >> >> >> and after where the method has it's own javadoc, the main description is >> added and the `@since` tags are added as intended. >> >> I don't have an account on `https://cr.openjdk.org/` but I could host the >> generated docs somewhere if that is needed. >> >> > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043;> >> >> > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b;> >> >> > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8;> >> >> >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Add new line when having multi-line doc comment Marked as reviewed by naoto (Reviewer). src/java.base/share/classes/java/text/ChoiceFormat.java line 591: > 589: /** > 590: * @since 23 > 591: */ IIUC, this addition will add & inherit the javadoc from `NumberFormat`, which is not the case before. The description for NumberFormat does not fit with ChoiceFormat. Probably that needs to be addressed with a different issue. - PR Review: https://git.openjdk.org/jdk/pull/18954#pullrequestreview-2142589023 PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1655299166
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v20]
On Wed, 26 Jun 2024 14:55:44 GMT, Shaojin Wen wrote:
>> The current versions of FloatToDecimal and DoubleToDecimal allocate
>> additional objects. Reducing these allocations can improve the performance
>> of Float/Double.toString and AbstractStringBuilder's append(float/double).
>>
>> This patch is just a code refactoring to reduce object allocation, but does
>> not change the Float/Double to decimal algorithm.
>>
>> The following code comments the allocated objects to be removed.
>>
>>
>> class FloatToDecimal {
>> public static String toString(float v) {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(float v, Appendable app)
>> throws IOException {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(float v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> if (app instanceof StringBuilder builder) {
>> return builder.append(chars);
>> }
>> if (app instanceof StringBuffer buffer) {
>> return buffer.append(chars);
>> }
>> for (char c : chars) {
>> app.append(c);
>> }
>> return app;
>> // ...
>> }
>> }
>> }
>>
>> class DoubleToDecimal {
>> public static String toString(double v) {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(double v, Appendable app)
>> throws IOException {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(double v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v, null)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> ...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. revert code change.
> 2. comment remove space
Changes requested by liach (Committer).
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 194:
> 192: * otherwise NON_SPECIAL
> 193: */
> 194: private int toDecimal(byte[] str, int index, double v,
> FormattedFPDecimal fd) {
This `toDecimal` returns packed int with info but the other returns the new
index; this is extremely confusing for future readers, especially that they
have very similar signatures (difference in signature is in the middle). I
recommend making this return a `long` so when it's downcasted to an int, it's
just the size (like String concat's coderIndex)
-
PR Review: https://git.openjdk.org/jdk/pull/19730#pullrequestreview-2142568839
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1655287303
Re: RFR: 8335060: ClassCastException after JDK-8294960
On Wed, 26 Jun 2024 06:53:28 GMT, Adam Sotona wrote: > Conversion of `java.lang.invoke` package to Class-File API is failing to > execute method handles with specific type conversion requirements. Root cause > is in the new `TypeConvertingMethodAdapter::primitiveTypeKindFromClass` > implementation. Original code has been matching the types by hash code and it > mistakenly appeared to be matching the primitive types. > > This patch fixes `TypeConvertingMethodAdapter::primitiveTypeKindFromClass` > and adds tests to better cover `TypeConvertingMethodAdapter` functionality. > > Please review. > > Thanks, > Adam Marked as reviewed by liach (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19898#pullrequestreview-2142548635
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v20]
On Wed, 26 Jun 2024 14:55:44 GMT, Shaojin Wen wrote:
>> The current versions of FloatToDecimal and DoubleToDecimal allocate
>> additional objects. Reducing these allocations can improve the performance
>> of Float/Double.toString and AbstractStringBuilder's append(float/double).
>>
>> This patch is just a code refactoring to reduce object allocation, but does
>> not change the Float/Double to decimal algorithm.
>>
>> The following code comments the allocated objects to be removed.
>>
>>
>> class FloatToDecimal {
>> public static String toString(float v) {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(float v, Appendable app)
>> throws IOException {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(float v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> if (app instanceof StringBuilder builder) {
>> return builder.append(chars);
>> }
>> if (app instanceof StringBuffer buffer) {
>> return buffer.append(chars);
>> }
>> for (char c : chars) {
>> app.append(c);
>> }
>> return app;
>> // ...
>> }
>> }
>> }
>>
>> class DoubleToDecimal {
>> public static String toString(double v) {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(double v, Appendable app)
>> throws IOException {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(double v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v, null)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> ...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. revert code change.
> 2. comment remove space
LGTM
-
Marked as reviewed by rgiulietti (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19730#pullrequestreview-2142544887
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v6]
On Wed, 26 Jun 2024 17:07:27 GMT, Nizar Benalla wrote: >> Please review this PR that aims to add all the remaining needed `@since` >> tags in `java.base`, and group them into a single fix. >> This is related to #18934 and my work around the `@since` checker feature. >> Explicit `@since` tags are needed for some overriding methods for the >> purpose of the checker. >> >> I will only give the example with the `CompletableFuture` class but here is >> the before where the methods only appeared in "Methods declared in interface >> N" >> >> > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac;> >> >> >> >> and after where the method has it's own javadoc, the main description is >> added and the `@since` tags are added as intended. >> >> I don't have an account on `https://cr.openjdk.org/` but I could host the >> generated docs somewhere if that is needed. >> >> > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043;> >> >> > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b;> >> >> > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8;> >> >> >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Add new line when having multi-line doc comment Marked as reviewed by liach (Committer). - PR Review: https://git.openjdk.org/jdk/pull/18954#pullrequestreview-2142538134
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v5]
On Wed, 26 Jun 2024 12:36:17 GMT, Chen Liang wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'upstream/master' into 8330954
>> - Merge remote-tracking branch 'upstream/master' into 8330954
>> - (C)
>> - (C)
>> - Move security classes changes to pr18373
>> - remove couple extra lines
>> - Pull request is now only about `@since` tags, might add an other commit
>> - add one more `{inheritDoc}` to `CompletableFuture.state`
>> - add `@throws` declarations to javadoc
>> - add ``{@inheritDoc}`` to new javadoc comments
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/82b51c65...dbef517a
>
> src/java.base/share/classes/java/lang/classfile/ClassSignature.java line 44:
>
>> 42: List typeParameters();
>> 43:
>> 44: /** {@return the instantiation of the superclass in this signature}
>
> Suggestion:
>
> /**
> * {@return the instantiation of the superclass in this signature}
>
> I think this is our preference if we have multi-line specs.
Fixed in
[91df97f](https://github.com/openjdk/jdk/pull/18954/commits/91df97f7661774bf4f262260e0732507399bce68),
Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1655253281
Integrated: 8333755: NumberFormat integer only parsing breaks when format has suffix
On Tue, 11 Jun 2024 17:25:29 GMT, Justin Lu wrote:
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8333920)
> which corrects a bug where NumberFormat cannot successfully parse values in
> integer only mode, when the format expects a suffix.
>
> For example,
>
> // a format that expects a currency suffix
> var fmt = NumberFormat.getCurrencyInstance(Locale.FRANCE);
> fmt.setParseIntegerOnly(true);
> failFmt.parse("5,00 €"); // throws ParseException when you would have
> expected 5 returned
>
>
> When parsing in integer only mode, instead of breaking upon a decimal symbol
> encounter, we should store the index but continue to fully parse so that we
> can verify the entire string and increment our position to search for and
> match the suffix. Upon a successful suffix match, we can then set
> `ParsePosition.index` back to the stored decimal index.
>
> It should be noted that CompactNumberFormat did previously have code that
> would traverse the rest of the String, to allow matching of the suffix. The
> difference is that NumberFormat returns the index upon decimal symbol
> encounter, while CompactNumberFormat was implemented to return the index
> reflected by the entire string traversal. Such differences cannot be
> standardized due to behavioral compatibility concerns.
>
> Thus while parsing in integer only, CNF sets index to the
> `Position.fullPos()`, while DF sets index to the `Position.intPos()`.
This pull request has now been integrated.
Changeset: bffc8484
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/bffc8484c32ad6c3205f7cebe4e262a2dc9de57e
Stats: 204 lines in 5 files changed: 102 ins; 34 del; 68 mod
8333755: NumberFormat integer only parsing breaks when format has suffix
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/19664
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v6]
> Please review this PR that aims to add all the remaining needed `@since` tags > in `java.base`, and group them into a single fix. > This is related to #18934 and my work around the `@since` checker feature. > Explicit `@since` tags are needed for some overriding methods for the purpose > of the checker. > > I will only give the example with the `CompletableFuture` class but here is > the before where the methods only appeared in "Methods declared in interface > N" > > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac;> > > > > and after where the method has it's own javadoc, the main description is > added and the `@since` tags are added as intended. > > I don't have an account on `https://cr.openjdk.org/` but I could host the > generated docs somewhere if that is needed. > > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043;> > > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b;> > > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8;> > > > TIA Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision: Add new line when having multi-line doc comment - Changes: - all: https://git.openjdk.org/jdk/pull/18954/files - new: https://git.openjdk.org/jdk/pull/18954/files/dbef517a..91df97f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18954=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18954=04-05 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18954.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18954/head:pull/18954 PR: https://git.openjdk.org/jdk/pull/18954
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v18]
On Wed, 26 Jun 2024 14:22:03 GMT, Raffaello Giulietti wrote: >> I looked at it again and I think @cl4es 's suggestion is correct. Can I >> submit the change? @rgiulietti > > Of course, nothing has been approved as of now. > > Since you are preparing a commit anyway, may I ask you to revert back the > changes > [here](https://github.com/openjdk/jdk/pull/19730#discussion_r1652978056)? > Thanks. I have completed all the suggested changes. Please continue to review. - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1655132555
Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v2]
> Hi all, > Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails on > rpm build mock environment. The `df -h` command return fail `df: cannot read > table of mounted file systems: No such file or directory` on the rpm build > mock environment also. I think it's a environmental issue, and the > environmental issue should not cause the test fails, it should skip the test. > > Only change the testcase, the change has been verified locally, no risk. SendaoYan has updated the pull request incrementally with one additional commit since the last revision: add a word throw - Changes: - all: https://git.openjdk.org/jdk/pull/19905/files - new: https://git.openjdk.org/jdk/pull/19905/files/54ac1747..8931debe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19905=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19905=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19905.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19905/head:pull/19905 PR: https://git.openjdk.org/jdk/pull/19905
Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment
On Wed, 26 Jun 2024 12:15:33 GMT, SendaoYan wrote: > Hi all, > Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails on > rpm build mock environment. The `df -h` command return fail `df: cannot read > table of mounted file systems: No such file or directory` on the rpm build > mock environment also. I think it's a environmental issue, and the > environmental issue should not cause the test fails, it should skip the test. > > Only change the testcase, the change has been verified locally, no risk. The GHA test runner report a failure, I think it's unrelated to this PR. 1. linux x86 fastdebug run test `compiler/interpreter/Test6833129.java` crash `oopDesc::size_given_klass`, seems similar to [JDK-8334760](https://bugs.openjdk.org/browse/JDK-8334760) - PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2191995696
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v20]
> The current versions of FloatToDecimal and DoubleToDecimal allocate
> additional objects. Reducing these allocations can improve the performance of
> Float/Double.toString and AbstractStringBuilder's append(float/double).
>
> This patch is just a code refactoring to reduce object allocation, but does
> not change the Float/Double to decimal algorithm.
>
> The following code comments the allocated objects to be removed.
>
>
> class FloatToDecimal {
> public static String toString(float v) {
> // allocate object FloatToDecimal
> return new FloatToDecimal().toDecimalString(v);
> }
>
> public static Appendable appendTo(float v, Appendable app)
> throws IOException {
> // allocate object FloatToDecimal
> return new FloatToDecimal().appendDecimalTo(v, app);
> }
>
> private Appendable appendDecimalTo(float v, Appendable app)
> throws IOException {
> switch (toDecimal(v)) {
> case NON_SPECIAL:
> // allocate object char[]
> char[] chars = new char[index + 1];
> for (int i = 0; i < chars.length; ++i) {
> chars[i] = (char) bytes[i];
> }
> if (app instanceof StringBuilder builder) {
> return builder.append(chars);
> }
> if (app instanceof StringBuffer buffer) {
> return buffer.append(chars);
> }
> for (char c : chars) {
> app.append(c);
> }
> return app;
> // ...
> }
> }
> }
>
> class DoubleToDecimal {
> public static String toString(double v) {
> // allocate object DoubleToDecimal
> return new DoubleToDecimal(false).toDecimalString(v);
> }
>
> public static Appendable appendTo(double v, Appendable app)
> throws IOException {
> // allocate object DoubleToDecimal
> return new DoubleToDecimal(false).appendDecimalTo(v, app);
> }
>
> private Appendable appendDecimalTo(double v, Appendable app)
> throws IOException {
> switch (toDecimal(v, null)) {
> case NON_SPECIAL:
> // allocate object char[]
> char[] chars = new char[index + 1];
> for (int i = 0; i < chars.length; ++i) {
> chars[i] = (char) bytes[i];
> }
> if (app instanceof StringBuilder builder) {
> return builder.append(chars);
> }
> ...
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
1. revert code change.
2. comment remove space
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/a3d216dd..b4f2d875
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19730=19
- incr: https://webrevs.openjdk.org/?repo=jdk=19730=18-19
Stats: 7 lines in 2 files changed: 0 ins; 3 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/19730.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19730/head:pull/19730
PR: https://git.openjdk.org/jdk/pull/19730
Re: RFR: 8334437: De-duplicate ProxyMethod list creation
On Tue, 18 Jun 2024 07:41:26 GMT, Claes Redestad wrote: > Simple refactoring to extract identical, simple lambda expressions. Improve > code clarity and reduce classes generated. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19760#issuecomment-2191894639
Integrated: 8334437: De-duplicate ProxyMethod list creation
On Tue, 18 Jun 2024 07:41:26 GMT, Claes Redestad wrote: > Simple refactoring to extract identical, simple lambda expressions. Improve > code clarity and reduce classes generated. This pull request has now been integrated. Changeset: 5883a20b Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/5883a20b822bb8acb719076e4f7abee8403061cb Stats: 10 lines in 1 file changed: 4 ins; 4 del; 2 mod 8334437: De-duplicate ProxyMethod list creation Reviewed-by: asotona, liach - PR: https://git.openjdk.org/jdk/pull/19760
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v19]
> The current versions of FloatToDecimal and DoubleToDecimal allocate
> additional objects. Reducing these allocations can improve the performance of
> Float/Double.toString and AbstractStringBuilder's append(float/double).
>
> This patch is just a code refactoring to reduce object allocation, but does
> not change the Float/Double to decimal algorithm.
>
> The following code comments the allocated objects to be removed.
>
>
> class FloatToDecimal {
> public static String toString(float v) {
> // allocate object FloatToDecimal
> return new FloatToDecimal().toDecimalString(v);
> }
>
> public static Appendable appendTo(float v, Appendable app)
> throws IOException {
> // allocate object FloatToDecimal
> return new FloatToDecimal().appendDecimalTo(v, app);
> }
>
> private Appendable appendDecimalTo(float v, Appendable app)
> throws IOException {
> switch (toDecimal(v)) {
> case NON_SPECIAL:
> // allocate object char[]
> char[] chars = new char[index + 1];
> for (int i = 0; i < chars.length; ++i) {
> chars[i] = (char) bytes[i];
> }
> if (app instanceof StringBuilder builder) {
> return builder.append(chars);
> }
> if (app instanceof StringBuffer buffer) {
> return buffer.append(chars);
> }
> for (char c : chars) {
> app.append(c);
> }
> return app;
> // ...
> }
> }
> }
>
> class DoubleToDecimal {
> public static String toString(double v) {
> // allocate object DoubleToDecimal
> return new DoubleToDecimal(false).toDecimalString(v);
> }
>
> public static Appendable appendTo(double v, Appendable app)
> throws IOException {
> // allocate object DoubleToDecimal
> return new DoubleToDecimal(false).appendDecimalTo(v, app);
> }
>
> private Appendable appendDecimalTo(double v, Appendable app)
> throws IOException {
> switch (toDecimal(v, null)) {
> case NON_SPECIAL:
> // allocate object char[]
> char[] chars = new char[index + 1];
> for (int i = 0; i < chars.length; ++i) {
> chars[i] = (char) bytes[i];
> }
> if (app instanceof StringBuilder builder) {
> return builder.append(chars);
> }
> ...
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
1. revert code change.
2. use boolean latin1.
3. replace `index++` to `index = `
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/5d3405f9..a3d216dd
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19730=18
- incr: https://webrevs.openjdk.org/?repo=jdk=19730=17-18
Stats: 34 lines in 3 files changed: 0 ins; 9 del; 25 mod
Patch: https://git.openjdk.org/jdk/pull/19730.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19730/head:pull/19730
PR: https://git.openjdk.org/jdk/pull/19730
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null [v4]
On Wed, 26 Jun 2024 14:38:04 GMT, Claes Redestad wrote: >> @wenshao @cl4es I think this use-case is quite a valid one, and deserves an >> extension. I filed: >> >> [JDK-8335113](https://bugs.openjdk.org/browse/JDK-8335113): C2 MergeStores: >> allow merging of putChar and other larger stores on smaller array elements > >> I filed: >> >> [JDK-8335113](https://bugs.openjdk.org/browse/JDK-8335113): C2 MergeStores: >> allow merging of putChar and other larger stores on smaller array elements > > Great! Should we hold off on this optimization and see if we can avoid the > need for `Unsafe` here - or go ahead integrating this PR and leave it to > JDK-8335113 to revert any no-longer-needed changes made here? @cl4es I would wait, to be honest. I'm currently quite busy, but I hope I can do something here in the next 2-3 weeks. - PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2191882317
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null [v4]
On Tue, 25 Jun 2024 17:19:28 GMT, Emanuel Peter wrote: > I filed: > > [JDK-8335113](https://bugs.openjdk.org/browse/JDK-8335113): C2 MergeStores: > allow merging of putChar and other larger stores on smaller array elements Great! Should we hold off on this optimization and see if we can avoid the need for `Unsafe` here - or go ahead integrating this PR and leave it to JDK-8335113 to revert any no-longer-needed changes made here? - PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2191878373
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v18]
On Wed, 26 Jun 2024 14:17:47 GMT, Shaojin Wen wrote: >> Yeah, I'm just musing about the perils of leaking/replicating those >> implementation details, but as you say this is internal code only used by >> `String` and `StringBuilder` to it's loosely part of that complex. Not >> requesting any changes here for now.. > > I looked at it again and I think @cl4es 's suggestion is correct. Can I > submit the change? @rgiulietti Of course, nothing has been approved as of now. Since you are preparing a commit anyway, may I ask you to revert back the changes [here](https://github.com/openjdk/jdk/pull/19730#discussion_r1652978056)? Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654972151
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v18]
On Wed, 26 Jun 2024 14:00:36 GMT, Claes Redestad wrote: >> I have written a version using boolean locally, but because this class is >> mainly used by String and StringBuilder, it uses the same style as >> String/StringBuilder. > > Yeah, I'm just musing about the perils of leaking/replicating those > implementation details, but as you say this is internal code only used by > `String` and `StringBuilder` to it's loosely part of that complex. Not > requesting any changes here for now.. I looked at it again and I think @cl4es 's suggestion is correct. Can I submit the change? @rgiulietti - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654964989
Re: RFR: 8334441: Mark tests in jdk_security_infra group as manual [v2]
On Tue, 25 Jun 2024 17:45:23 GMT, Andrew John Hughes wrote: > Is there still going to be some monitoring of these tests? Making it harder > to run these tests potentially means that genuine failures can go unnoticed > and JDK certificates are not updated when needed. Tracking won’t be any different than other manual tests in the test suite. Within Oracle, we recommend that developers working on security libraries fixes run these tests before integration. All manual tests are definitely run during the release certification. - PR Comment: https://git.openjdk.org/jdk/pull/19814#issuecomment-2191822556
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v18]
On Wed, 26 Jun 2024 13:23:14 GMT, Shaojin Wen wrote: >> src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 47: >> >>> 45: static final int NAN = 5 << 8; >>> 46: >>> 47: static final byte LATIN1 = 0; >> >> I think this somewhat unnecessarily copies names and internal implementation >> details from `String` (where the `coder` value is used both as a >> pseudo-boolean and as a shift operand, which is not applicable here). In >> this context we could use something like `private final boolean latin1;` >> (all uses of `coder` are just `if (coder == LATIN1)` so it would be >> simplified to `if (latin1)`) > > I have written a version using boolean locally, but because this class is > mainly used by String and StringBuilder, it uses the same style as > String/StringBuilder. Yeah, I'm just musing about the perils of leaking/replicating those implementation details, but as you say this is internal code only used by `String` and `StringBuilder` to it's loosely part of that complex. Not requesting any changes here for now.. - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654924418
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
On Wed, 26 Jun 2024 13:20:58 GMT, Shaojin Wen wrote: >> In principle, you should not arbitrarily change code that is correct, and >> only limit your changes to reach the goal of the PR. >> My suggestion is the minimal change required, yours is more than strictly >> needed. > > Got it, should I change it back? If there's nothing else, no, there's no need to switch back. Sorry if I sound pedantic on some principles that I feel are important for the good health of OpenJDK ;-) - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654905660
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
On Wed, 26 Jun 2024 12:32:20 GMT, Jan Lahoda wrote:
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 336:
> 334: generateTypeSwitch(lookup, enumClass,
> labels)
> 335:
> .asType(MethodType.methodType(int.class,
> 336:
> enumClass,
We might have to do `Object.class` so we can call `invokeExact` below
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 426:
> 424: public volatile int[] constantsMap;
> 425: @Stable
> 426: public volatile MethodHandle generatedSwitch;
We probably don't need these 2 volatiles:
1. Arrays are already safely published (See
https://bugs.openjdk.org/browse/JDK-8333791?focusedId=14680594=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14680594)
and we can only access array elements after the array is fully initialized
then published. Thus it's a safe publication, and reads don't require explicit
volatile read.
2. `MethodHandle` is immutable and safely published, thus volatile read is
redundant as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654860151
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654773322
Re: RFR: 8334437: De-duplicate ProxyMethod list creation
On Tue, 18 Jun 2024 07:41:26 GMT, Claes Redestad wrote: > Simple refactoring to extract identical, simple lambda expressions. Improve > code clarity and reduce classes generated. Marked as reviewed by liach (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19760#pullrequestreview-2141860506
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v18]
On Wed, 26 Jun 2024 13:14:13 GMT, Claes Redestad wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> from @liach: use s.getBytes for performance > > src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 47: > >> 45: static final int NAN = 5 << 8; >> 46: >> 47: static final byte LATIN1 = 0; > > I think this somewhat unnecessarily copies names and internal implementation > details from `String` (where the `coder` value is used both as a > pseudo-boolean and as a shift operand, which is not applicable here). In this > context we could use something like `private final boolean latin1;` (all uses > of `coder` are just `if (coder == LATIN1)` so it would be simplified to `if > (latin1)`) I have written a version using boolean locally, but because this class is mainly used by String and StringBuilder, it uses the same style as String/StringBuilder. - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654832167
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
On Wed, 26 Jun 2024 12:50:38 GMT, Raffaello Giulietti wrote: >> I like the new implementation, the code is cleaner, is your suggestion to >> revert to the original version due to smaller changes? > > In principle, you should not arbitrarily change code that is correct, and > only limit your changes to reach the goal of the PR. > My suggestion is the minimal change required, yours is more than strictly > needed. Got it, should I change it back? - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654828664
Re: RFR: 8334562: Automate com/sun/security/auth/callback/TextCallbackHandler/Default.java test
On Wed, 19 Jun 2024 12:47:33 GMT, Fernando Guallini wrote: > The following test: > **com/sun/security/auth/callback/TextCallbackHandler/Default.java** is > currently marked to be run manually because user inputs are required in the > console, but instead it can be automated by providing a custom inputStream to > System.in in the actual test to simulate sequential user input. > > In addition, this patch is removing the test from the problemList as it > passes, and from manual test list. Please, I would need a review on this PR - PR Comment: https://git.openjdk.org/jdk/pull/19790#issuecomment-2191674728
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v18]
On Tue, 25 Jun 2024 17:29:04 GMT, Shaojin Wen wrote:
>> The current versions of FloatToDecimal and DoubleToDecimal allocate
>> additional objects. Reducing these allocations can improve the performance
>> of Float/Double.toString and AbstractStringBuilder's append(float/double).
>>
>> This patch is just a code refactoring to reduce object allocation, but does
>> not change the Float/Double to decimal algorithm.
>>
>> The following code comments the allocated objects to be removed.
>>
>>
>> class FloatToDecimal {
>> public static String toString(float v) {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(float v, Appendable app)
>> throws IOException {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(float v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> if (app instanceof StringBuilder builder) {
>> return builder.append(chars);
>> }
>> if (app instanceof StringBuffer buffer) {
>> return buffer.append(chars);
>> }
>> for (char c : chars) {
>> app.append(c);
>> }
>> return app;
>> // ...
>> }
>> }
>> }
>>
>> class DoubleToDecimal {
>> public static String toString(double v) {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(double v, Appendable app)
>> throws IOException {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(double v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v, null)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> ...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> from @liach: use s.getBytes for performance
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 47:
> 45: static final int NAN = 5 << 8;
> 46:
> 47: static final byte LATIN1 = 0;
I think this somewhat unnecessarily copies names and internal implementation
details from `String` (where the `coder` value is used both as a pseudo-boolean
and as a shift operand, which is not applicable here). In this context we could
use something like `private final boolean latin1;` (all uses of `coder` are
just `if (coder == LATIN1)` so it would be simplified to `if (latin1)`)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654812101
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
On Wed, 26 Jun 2024 12:46:18 GMT, Claes Redestad wrote:
>> For general pattern matching switches, the `SwitchBootstraps` class
>> currently generates a cascade of `if`-like statements, computing the correct
>> target case index for the given input.
>>
>> There is one special case which permits a relatively easy faster handling,
>> and that is when all the case labels case enum constants (but the switch is
>> still a "pattern matching" switch, as tranditional enum switches do not go
>> through `SwitchBootstraps`). Like:
>>
>>
>> enum E {A, B, C}
>> E e = ...;
>> switch (e) {
>> case null -> {}
>> case A a -> {}
>> case C c -> {}
>> case B b -> {}
>> }
>>
>>
>> We can create an array mapping the runtime ordinal to the appropriate case
>> number, which is somewhat similar to what javac is doing for ordinary
>> switches over enums.
>>
>> The `SwitchBootstraps` class was trying to do so, when the restart index is
>> zero, but failed to do so properly, so that code is not used (and does not
>> actually work properly).
>>
>> This patch is trying to fix that - when all the case labels are enum
>> constants, an array mapping the runtime enum ordinals to the case number
>> will be created (lazily), for restart index == 0. And this map will then be
>> used to quickly produce results for the given input. E.g. for the case
>> above, the mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B
>> -> 2, C -> 1}`).
>>
>> When the restart index is != 0 (i.e. when there's a guard in the switch, and
>> the guard returned `false`), the if cascade will be generated lazily and
>> used, as in the general case. If it would turn out there are significant
>> enum-only switches with guards/restart index != 0, we could improve there as
>> well, by generating separate mappings for every (used) restart index.
>>
>> I believe the current tests cover the code functionally sufficiently - see
>> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
>> regression tests cannot easily, I think) differentiate whether the
>> special-case or generic implementation is used.
>>
>> I've added a new microbenchmark attempting to demonstrate the difference.
>> There are two benchmarks, both having only enum constants as case labels.
>> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
>> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
>> `SwitchBootstraps`. Before this patch, I was getting values like:
>>
>> Benchmark Mode Cnt Score Error Units
>> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
>> Swi...
>
> test/micro/org/openjdk/bench/java/lang/runtime/SwitchEnum.java line 57:
>
>> 55: for (E e : inputs) {
>> 56: sum += switch (e) {
>> 57: case null -> -1;
>
> As this `null` case adds a case relative to the `-Traditional` test then
> maybe removing one of the `E0, E1, ...` cases would make the test a little
> bit more apples-to-apples?
Using `case null -> ` will push javac to use the new code, but all switches do
some kind of null check for the selector value. The difference is that if
there's no `case null`, there will be `Objects.requireNonNull` generated for
the selector value (which will throw an NPE if the value is `null`), while here
it will jump to the given case.
So, `case null` does not have the same weight as a normal case, so I don't
think it would be fair to remove a full case to compensate for it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654784712
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
On Wed, 26 Jun 2024 12:32:20 GMT, Jan Lahoda wrote:
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
Marked as reviewed by redestad (Reviewer).
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 280:
> 278:
> 279: Class enumClass = invocationType.parameterType(0);
> 280: labels = Stream.of(labels).map(l -> convertEnumConstants(lookup,
> enumClass, l)).toArray();
You could create `EnumDesc[] enumDescLabels` here and remove the
`Arrays.copyOf` on line 290. The `labels.clone()` on line 277 also seem
redundant since we only iterate over the `labels` argument once.
While this case is likely fine I generally recommend using a minimal amount of
streams/lambdas in bootstrap sensitive code like these dynamic bootstraps
methods.
-
PR Review: https://git.openjdk.org/jdk/pull/19906#pullrequestreview-2141731748
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654771207
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
On Tue, 25 Jun 2024 17:02:10 GMT, Shaojin Wen wrote: >> src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 236: >> >>> 234: dk = -1; >>> 235: } >>> 236: return toDecimal(str, index, Q_MIN, t, dk, fd) - start; >> >> I suggest restoring the original logic like so: >> >> /* subnormal value */ >> return (t < C_TINY >> ? toDecimal(str, index, Q_MIN, 10 * t, -1, fd) >> : toDecimal(str, index, Q_MIN, t, 0, fd)) - start; > > I like the new implementation, the code is cleaner, is your suggestion to > revert to the original version due to smaller changes? In principle, you should not arbitrarily change code that is correct, and only limit your changes to reach the goal of the PR. My suggestion is the minimal change required, yours is more than strictly needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1654762216
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
On Wed, 26 Jun 2024 12:32:20 GMT, Jan Lahoda wrote:
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
test/micro/org/openjdk/bench/java/lang/runtime/SwitchEnum.java line 57:
> 55: for (E e : inputs) {
> 56: sum += switch (e) {
> 57: case null -> -1;
As this `null` case adds a case relative to the `-Traditional` test then maybe
removing one of the `E0, E1, ...` cases would make the test a little bit more
apples-to-apples?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654753822
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v5]
On Wed, 26 Jun 2024 10:08:38 GMT, Nizar Benalla wrote:
>> Please review this PR that aims to add all the remaining needed `@since`
>> tags in `java.base`, and group them into a single fix.
>> This is related to #18934 and my work around the `@since` checker feature.
>> Explicit `@since` tags are needed for some overriding methods for the
>> purpose of the checker.
>>
>> I will only give the example with the `CompletableFuture` class but here is
>> the before where the methods only appeared in "Methods declared in interface
>> N"
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac;>
>>
>>
>>
>> and after where the method has it's own javadoc, the main description is
>> added and the `@since` tags are added as intended.
>>
>> I don't have an account on `https://cr.openjdk.org/` but I could host the
>> generated docs somewhere if that is needed.
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043;>
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b;>
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8;>
>>
>>
>> TIA
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 12 additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into 8330954
> - Merge remote-tracking branch 'upstream/master' into 8330954
> - (C)
> - (C)
> - Move security classes changes to pr18373
> - remove couple extra lines
> - Pull request is now only about `@since` tags, might add an other commit
> - add one more `{inheritDoc}` to `CompletableFuture.state`
> - add `@throws` declarations to javadoc
> - add ``{@inheritDoc}`` to new javadoc comments
> - ... and 2 more: https://git.openjdk.org/jdk/compare/6e3c8053...dbef517a
Marked as reviewed by liach (Committer).
src/java.base/share/classes/java/lang/classfile/ClassSignature.java line 44:
> 42: List typeParameters();
> 43:
> 44: /** {@return the instantiation of the superclass in this signature}
Suggestion:
/**
* {@return the instantiation of the superclass in this signature}
I think this is our preference if we have multi-line specs.
-
PR Review: https://git.openjdk.org/jdk/pull/18954#pullrequestreview-2141680174
PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1654739868
RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
For general pattern matching switches, the `SwitchBootstraps` class currently
generates a cascade of `if`-like statements, computing the correct target case
index for the given input.
There is one special case which permits a relatively easy faster handling, and
that is when all the case labels case enum constants (but the switch is still a
"pattern matching" switch, as tranditional enum switches do not go through
`SwitchBootstraps`). Like:
enum E {A, B, C}
E e = ...;
switch (e) {
case null -> {}
case A a -> {}
case C c -> {}
case B b -> {}
}
We can create an array mapping the runtime ordinal to the appropriate case
number, which is somewhat similar to what javac is doing for ordinary switches
over enums.
The `SwitchBootstraps` class was trying to do so, when the restart index is
zero, but failed to do so properly, so that code is not used (and does not
actually work properly).
This patch is trying to fix that - when all the case labels are enum constants,
an array mapping the runtime enum ordinals to the case number will be created
(lazily), for restart index == 0. And this map will then be used to quickly
produce results for the given input. E.g. for the case above, the mapping will
be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C -> 1}`).
When the restart index is != 0 (i.e. when there's a guard in the switch, and
the guard returned `false`), the if cascade will be generated lazily and used,
as in the general case. If it would turn out there are significant enum-only
switches with guards/restart index != 0, we could improve there as well, by
generating separate mappings for every (used) restart index.
I believe the current tests cover the code functionally sufficiently - see
`SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
regression tests cannot easily, I think) differentiate whether the special-case
or generic implementation is used.
I've added a new microbenchmark attempting to demonstrate the difference. There
are two benchmarks, both having only enum constants as case labels. One,
`enumSwitchTraditional` is an "old" switch, desugared fully by javac, the
other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
`SwitchBootstraps`. Before this patch, I was getting values like:
Benchmark Mode Cnt Score Error Units
SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ns/op
and with this patch:
Benchmark Mode Cnt Score Error Units
SwitchEnum.enumSwitchTraditionalavgt 15 11.550 ± 0.157 ns/op
SwitchEnum.enumSwitchWithBootstrap avgt 15 13.225 ± 0.173 ns/op
So, this seems like a clear improvement to me.
-
Commit messages:
- 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
Changes: https://git.openjdk.org/jdk/pull/19906/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19906=00
Issue: https://bugs.openjdk.org/browse/JDK-8332522
Stats: 187 lines in 2 files changed: 149 ins; 18 del; 20 mod
Patch: https://git.openjdk.org/jdk/pull/19906.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19906/head:pull/19906
PR: https://git.openjdk.org/jdk/pull/19906
RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment
Hi all, Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails on rpm build mock environment. The `df -h` command return fail `df: cannot read table of mounted file systems: No such file or directory` on the rpm build mock environment also. I think it's a environmental issue, and the environmental issue should not cause the test fails, it should skip the test. Only change the testcase, the change has been verified locally, no risk. - Commit messages: - 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment Changes: https://git.openjdk.org/jdk/pull/19905/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19905=00 Issue: https://bugs.openjdk.org/browse/JDK-8335150 Stats: 23 lines in 1 file changed: 15 ins; 3 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19905.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19905/head:pull/19905 PR: https://git.openjdk.org/jdk/pull/19905
Re: RFR: 8334437: De-duplicate ProxyMethod list creation
On Tue, 18 Jun 2024 07:41:26 GMT, Claes Redestad wrote: > Simple refactoring to extract identical, simple lambda expressions. Improve > code clarity and reduce classes generated. Looks good to me. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19760#pullrequestreview-2141591212
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v8]
On Wed, 26 Jun 2024 11:30:25 GMT, Raffaello Giulietti wrote: >> All suggestions have been fixed, can this PR be integrated? @cl4es @liach > > @wenshao Looks good, thanks for the improvements. > > Let me know when you are finished with your changes. > Once approved, you should refrain from adding other commits without further > approvals. @rgiulietti Thanks for your review. I have completed all the work I wanted to do on this PR. I will not make any changes without further suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/19730#issuecomment-2191509819
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v8]
On Tue, 25 Jun 2024 01:58:29 GMT, Shaojin Wen wrote: >> Just suggesting some improvements > > All suggestions have been fixed, can this PR be integrated? @cl4es @liach @wenshao Looks good, thanks for the improvements. Let me know when you are finished with your changes. Once approved, you should refrain from adding other commits without further approvals. - PR Comment: https://git.openjdk.org/jdk/pull/19730#issuecomment-2191464714
Re: RFR: 8334437: De-duplicate ProxyMethod list creation
On Tue, 18 Jun 2024 07:41:26 GMT, Claes Redestad wrote: > Simple refactoring to extract identical, simple lambda expressions. Improve > code clarity and reduce classes generated. Friendly reminder that this trivial improvement needs a reviewer. - PR Comment: https://git.openjdk.org/jdk/pull/19760#issuecomment-2191352440
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps [v3]
On Tue, 18 Jun 2024 10:00:46 GMT, Claes Redestad wrote: >> 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 Friendly reminder that this improvement needs a Reviewer. While it'd be great if javac can make this redundant this is adding overhead in the meantime, and the temporary workaround doesn't obfuscate the code. - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2191356136
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v5]
> Please review this PR that aims to add all the remaining needed `@since` tags
> in `java.base`, and group them into a single fix.
> This is related to #18934 and my work around the `@since` checker feature.
> Explicit `@since` tags are needed for some overriding methods for the purpose
> of the checker.
>
> I will only give the example with the `CompletableFuture` class but here is
> the before where the methods only appeared in "Methods declared in interface
> N"
>
> src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac;>
>
>
>
> and after where the method has it's own javadoc, the main description is
> added and the `@since` tags are added as intended.
>
> I don't have an account on `https://cr.openjdk.org/` but I could host the
> generated docs somewhere if that is needed.
>
> src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043;>
>
> src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b;>
>
> src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8;>
>
>
> TIA
Nizar Benalla has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains 12 additional commits
since the last revision:
- Merge remote-tracking branch 'upstream/master' into 8330954
- Merge remote-tracking branch 'upstream/master' into 8330954
- (C)
- (C)
- Move security classes changes to pr18373
- remove couple extra lines
- Pull request is now only about `@since` tags, might add an other commit
- add one more `{inheritDoc}` to `CompletableFuture.state`
- add `@throws` declarations to javadoc
- add ``{@inheritDoc}`` to new javadoc comments
- ... and 2 more: https://git.openjdk.org/jdk/compare/5654cccb...dbef517a
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18954/files
- new: https://git.openjdk.org/jdk/pull/18954/files/b3574b97..dbef517a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18954=04
- incr: https://webrevs.openjdk.org/?repo=jdk=18954=03-04
Stats: 141869 lines in 2684 files changed: 97659 ins; 31514 del; 12696 mod
Patch: https://git.openjdk.org/jdk/pull/18954.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18954/head:pull/18954
PR: https://git.openjdk.org/jdk/pull/18954
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 25 Jun 2024 19:33:02 GMT, Naoto Sato wrote: > > So, considering all the information given, is it enough to start our new > > review process? @naotoj @liach @justin-curtis-lu > > Well, I was suggesting the same buffer proxying for other Format classes than > NumberFormat subclasses, such as DateFormat so that they would have the same > performance benefit. Would you be willing to do that too? Sure. All `Format`'s subclasses has been replaced with buffer proxying. After that I run the benchmark test with averageTime mode. The result show the StringBuilder has take effect. Please review again. @naotoj @liach @justin-curtis-lu | Testcase | JDK 11 | JDK 22 | Current JDK | | - | - |- | - | | java.text.NumberFormat#format(double)| 362.221 | 636.049 | 351.913| | java.text.DateFormat#format(java.util.Date)| 362.273|944.733|317.436| |java.text.MessageFormat#format| 599.146| 937.717|499.584| |java.text.ListFormat#format| N/A | 464.123|318.978| - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2191307113
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v7]
On Tue, 25 Jun 2024 13:54:46 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refactor mount info matching to helper function > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Remove problem listing of PlainRead which is reworked here > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Add doc for mountinfo scanning. > - Unify naming of variables > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - ... and 7 more: https://git.openjdk.org/jdk/compare/baafa662...532ea33b Could I get a second review on this please? @larry-cable maybe? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-219133
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
> ### 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/b6646c5d..7eb9b523
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19513=11
- incr: https://webrevs.openjdk.org/?repo=jdk=19513=10-11
Stats: 242 lines in 9 files changed: 220 ins; 0 del; 22 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: 8333308: javap --system handling doesn't work on internal class names
On Tue, 25 Jun 2024 13:59:35 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [JDK-808](https://bugs.openjdk.org/browse/JDK-808) > enabling `javap -system` to handle internal class names. > > Thanks, > Sonia For a test, I agree it is quite difficult. For the `--system` parameter, it would probably be possible to create a directory (e.g. `$DIR`), and inside it `$DIR/lib`, and copy suitable `jrt-fs.jar` and `modules` to it from `$JDK/lib`. And the use `--system $DIR`. But, it would still be fairly difficult to be sure the class' content is read from `$DIR`, not from the runtime JDK - `modules` would presumably need to be modified/different than the one for the runtime JDK, and that is not very easy. - PR Comment: https://git.openjdk.org/jdk/pull/19883#issuecomment-2191232755
Re: RFR: 8335110: Fix instruction name and API spec inconsistencies in CodeBuilder [v2]
On Tue, 25 Jun 2024 21:22:34 GMT, Chen Liang wrote: >> This is a collection of fixes and improvements to CodeBuilder, plus 2 >> renames. >> >> Fixes include: >> 1. `CodeBuilder::receiverSlot` typo >> 2. `CodeAttribute::labelToBci` update spec >> 3. `CodeBuilder::exceptionCatch` implementation >> 4. `CodeBuilder::if_nonnull`/`if_null` -> `ifnonnull`/`ifnull` >> 5. Docs for what instructions factories emit, and to explain why some >> factories have name mismatch; also a section in summary. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Patch in csr but forgot to upload Looks good to me, thank you. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19889#pullrequestreview-2141230227
Re: RFR: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960 [v3]
On Tue, 25 Jun 2024 13:47:39 GMT, Adam Sotona wrote: >> After JDK-8294960 is java.lang.invoke.ClassSpecializer using lamdas for code >> generation and unfortunately it causes StackOverflow on BigEndian platforms. >> >> This patch converts all lambdas in ClassSpecializer into anonymous inner >> classes. >> >> Please review and test on a BigEndian platform. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > more de-lambda work on ClassSpecializer Thank you for confirmation. - PR Comment: https://git.openjdk.org/jdk/pull/19863#issuecomment-2191198224
Integrated: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960
On Mon, 24 Jun 2024 16:01:41 GMT, Adam Sotona wrote: > After JDK-8294960 is java.lang.invoke.ClassSpecializer using lamdas for code > generation and unfortunately it causes StackOverflow on BigEndian platforms. > > This patch converts all lambdas in ClassSpecializer into anonymous inner > classes. > > Please review and test on a BigEndian platform. > > Thanks, > Adam This pull request has now been integrated. Changeset: 7f6804ce Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/7f6804ceb63568d72e825d45b02d08f314c9b0fc Stats: 290 lines in 1 file changed: 110 ins; 84 del; 96 mod 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960 Reviewed-by: redestad - PR: https://git.openjdk.org/jdk/pull/19863
Re: RFR: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960 [v3]
On Tue, 25 Jun 2024 13:47:39 GMT, Adam Sotona wrote: >> After JDK-8294960 is java.lang.invoke.ClassSpecializer using lamdas for code >> generation and unfortunately it causes StackOverflow on BigEndian platforms. >> >> This patch converts all lambdas in ClassSpecializer into anonymous inner >> classes. >> >> Please review and test on a BigEndian platform. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > more de-lambda work on ClassSpecializer The tests have passed on AIX, too. - PR Comment: https://git.openjdk.org/jdk/pull/19863#issuecomment-2191118572
Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Wed, 26 Jun 2024 07:54:16 GMT, Alan Bateman wrote: > > Question, what is the noreg-hard label used for? > > It's the label to mean that it's too hard or impossible write a regression > test. It is documented in the [JBS label > dictionary](https://openjdk.org/guide/#jbs-label-dictionary) but may not be > widely known. Ah, thank you, and I never knew this documentation either. - PR Comment: https://git.openjdk.org/jdk/pull/19883#issuecomment-2191089714
Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Wed, 26 Jun 2024 06:04:08 GMT, Thomas Stuefe wrote: > Question, what is the noreg-hard label used for? It's the label to mean that it's too hard or impossible write a regression test. It is documented in the [JBS label dictionary](https://openjdk.org/guide/#jbs-label-dictionary) but may not be widely known. - PR Comment: https://git.openjdk.org/jdk/pull/19883#issuecomment-2191053610
RFR: 8335060: ClassCastException after JDK-8294960
Conversion of `java.lang.invoke` package to Class-File API is failing to execute method handles with specific type conversion requirements. Root cause is in the new `TypeConvertingMethodAdapter::primitiveTypeKindFromClass` implementation. Original code has been matching the types by hash code and it mistakenly appeared to be matching the primitive types. This patch fixes `TypeConvertingMethodAdapter::primitiveTypeKindFromClass` and adds tests to better cover `TypeConvertingMethodAdapter` functionality. Please review. Thanks, Adam - Commit messages: - 8335060: ClassCastException after JDK-8294960 Changes: https://git.openjdk.org/jdk/pull/19898/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19898=00 Issue: https://bugs.openjdk.org/browse/JDK-8335060 Stats: 806 lines in 2 files changed: 798 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19898.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19898/head:pull/19898 PR: https://git.openjdk.org/jdk/pull/19898
Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Tue, 25 Jun 2024 19:49:07 GMT, Chen Liang wrote: > Technically javap accepts both notations of `a.b.C` and `a/b/C.class` and > accepts both `.` and `$` as inner class separators. So this is fine. However > it's hard to verify that the jdk in `--system` is really used, so I put a > noreg-hard label on the original JBS issue; it's hard to create a suitable > argument for the `--system` flag as you need a whole JDK. Question, what is the noreg-hard label used for? - PR Comment: https://git.openjdk.org/jdk/pull/19883#issuecomment-2190783227
