Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v6]
> 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:
add comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/d28e83a9..8a649c72
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=04-05
Stats: 18 lines in 2 files changed: 18 ins; 0 del; 0 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: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v5]
> 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:
remove unused imports
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/761e1087..d28e83a9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=03-04
Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 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: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v4]
On Sat, 15 Jun 2024 02:45:40 GMT, Chen Liang wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> use JLA replace Unsafe
>
> src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 41:
>
>> 39: public sealed class ToDecimal permits DoubleToDecimal, FloatToDecimal{
>> 40: static final byte LATIN1 = 0;
>> 41: static final byte UTF16 = 1;
>
> @cl4es All these string latin1 vs utf16 infrastructure are duplicates of
> those in StringLatin1 and StringUTF16 in java.lang. Should we move those
> String internals to jdk.internal packages so it's more accessible to such
> coder-aware special optimizations, like we did with ConstantDesc?
I have used JavaLangAccess instead of Usnafe
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1640769814
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v4]
> 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:
use JLA replace Unsafe
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/95097e1f..761e1087
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=02-03
Stats: 33 lines in 1 file changed: 1 ins; 24 del; 8 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: 8333962: Obsolete OldSize [v2]
On Fri, 14 Jun 2024 10:19:47 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > obsolete-old-size Marked as reviewed by dholmes (Reviewer). src/hotspot/share/runtime/arguments.cpp line 37: > 35: #include "gc/shared/gcArguments.hpp" > 36: #include "gc/shared/gcConfig.hpp" > 37: #include "gc/shared/genArguments.hpp" Why is this needed? - PR Review: https://git.openjdk.org/jdk/pull/19647#pullrequestreview-2120074670 PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1640761871
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v3]
> 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:
add final modifier & comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/b7c3586e..95097e1f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=01-02
Stats: 15 lines in 2 files changed: 10 ins; 3 del; 2 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: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v2]
> 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:
Update src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java
Co-authored-by: Chen Liang
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/4c810154..b7c3586e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=00-01
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal
On Sat, 15 Jun 2024 03:02:19 GMT, Chen Liang 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];
>> }
>> ...
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 888:
>
>> 886: */
>> 887: public AbstractStringBuilder append(float f) {
>> 888: ensureCapacityInternal(count + FloatToDecimal.MAX_CHARS);
>
> This might cause failure in edge cases where the array size reaches the
> limit. Might need some review on how big an impact this would be.
The maximum length of float to decimal is 15, and the maximum length of double
to decimal is 24. The original exception handling was because the
Appendable#append(char) method threw an IOException, which is no longer
necessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1640755241
Re: RFR: 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);
> }
> ...
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 888:
> 886: */
> 887: public AbstractStringBuilder append(float f) {
> 888: ensureCapacityInternal(count + FloatToDecimal.MAX_CHARS);
This might cause failure in edge cases where the array size reaches the limit.
Might need some review on how big an impact this would be.
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 41:
> 39: public final class DoubleToDecimal extends ToDecimal {
> 40: public static DoubleToDecimal LATIN1 = new
> DoubleToDecimal(ToDecimal.LATIN1);
> 41: public static DoubleToDecimal UTF16 = new
> DoubleToDecimal(ToDecimal.UTF16);
Suggestion:
public static final DoubleToDecimal LATIN1 = new
DoubleToDecimal(ToDecimal.LATIN1);
public static final DoubleToDecimal UTF16 = new
DoubleToDecimal(ToDecimal.UTF16);
For constant folding.
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 44:
> 42:
> 43: /*
> 44: * For full details about this code see the following references:
We should copy these comments to `ToDecimal`; otherwise, it's not obvious what
references are being made in comments there.
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 41:
> 39: public sealed class ToDecimal permits DoubleToDecimal, FloatToDecimal{
> 40: static final byte LATIN1 = 0;
> 41: static final byte UTF16 = 1;
@cl4es All these string latin1 vs utf16 infrastructure are duplicates of those
in StringLatin1 and StringUTF16 in java.lang. Should we move those String
internals to jdk.internal packages so it's more accessible to such coder-aware
special optimizations, like we did with ConstantDesc?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1640634700
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1640634754
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1640742561
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1640632467
Re: RFR: 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);
> }
> ...
The performance numbers under `MacBookPro M1 Pro` & `MacBookPro 2018 i9` are as
follows:
## Apple MacBookPro 2018
* CPU Intel i9 (x64)
-Benchmark Mode CntScore Error Units
-StringBuilders.appendWithFloat8Latin1 avgt 15 381.206 ? 8.681 ns/op
-StringBuilders.appendWithFloat8Utf16avgt 15 366.549 ? 1.775 ns/op
-StringBuilders.appendWithDouble8Latin1 avgt 15 570.085 ? 8.040 ns/op
-StringBuilders.appendWithDouble8Utf16 avgt 15 501.409 ? 5.750 ns/op
+Benchmark Mode CntScoreError Units
(Webrevs 00 4c810154)
+StringBuilders.appendWithFloat8Latin1 avgt 15 226.236 ? 3.350 ns/op
+68.49%
+StringBuilders.appendWithFloat8Utf16avgt 15 299.072 ? 19.895 ns/op
+22.56%
+StringBuilders.appendWithDouble8Latin1 avgt 15 328.887 ? 5.865 ns/op
+73.33%
+StringBuilders.appendWithDouble8Utf16 avgt 15 390.618 ? 30.142 ns/op
+28.36%
## MacBook M1 Pro
* CPU Apple M1 Pro (aarch64)
-Benchmark Mode CntScoreError Units base
-StringBuilders.appendWithFloat8Latin1 avgt 15 317.144 ? 11.325 ns/op
-StringBuilders.appendWithFloat8Utf16avgt 15 316.980 ? 17.955 ns/op
-StringBuilders.appendWithDouble8Latin1 avgt 15 440.853 ? 13.067 ns/op
-StringBuilders.appendWithDouble8Utf16 avgt 15 418.896 ? 4.610 ns/op
+Benchmark Mode CntScoreError Units
(Webrevs 00 4c810154)
+StringBuilders.appendWithFloat8Latin1 avgt 15 168.231 ? 4.749 ns/op
+88.51%
+StringBuilders.appendWithFloat8Utf16avgt 15 213.981 ? 3.274 ns/op
+48.13%
+StringBuilders.appendWithDouble8Latin1 avgt 15 241.536 ? 0.993 ns/op
+82.52%
+StringBuilders.appendWithDouble8Utf16 avgt 15 284.863 ? 10.381 ns/op
+47.05%
-
PR Comment: https://git.openjdk.org/jdk/pull/19730#issuecomment-2169061500
RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal
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);
}
if (app instanceof StringBuffer buffer) {
return buffer.append(chars);
}
for (char c : chars) {
app.append(c);
}
return app;
// ...
}
}
}
-
Commit messages:
- Reduce object allocation for FloatToDecimal and DoubleToDecimal
Changes: https://git.openjdk.org/jdk/pull/19730/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8334328
Stats: 641 lines in 5 files changed: 293 ins; 211 del; 137 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: 8333268: Fixes for static build
On Thu, 30 May 2024 13:00:21 GMT, Magnus Ihse Bursie wrote: > This patch contains a set of changes to improve static builds. They will pave > the way for implementing a full static-only java launcher. The changes here > will: > > 1) Make sure non-exported symbols are made local in the static libraries. > This means that the risk of symbol conflict is the same for static libraries > as for dynamic libraries (i.e. in practice zero, as long as a consistent > naming scheme is used for exported functions). > > 2) Remove the work-arounds to exclude duplicated symbols. > > 3) Fix some code in hotspot and the JDK libraries that did not work properly > with a static java launcher. > > The latter fixes are copied from or inspired by the work done by @jianglizhou > and her team as part of the Project Leyden [Hermetic > Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). The GHA tests fails when building gtest on Linux. This will require some investigation. - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2168647325
Re: RFR: 8333268: Fixes for static build
On Thu, 30 May 2024 13:00:21 GMT, Magnus Ihse Bursie wrote:
> This patch contains a set of changes to improve static builds. They will pave
> the way for implementing a full static-only java launcher. The changes here
> will:
>
> 1) Make sure non-exported symbols are made local in the static libraries.
> This means that the risk of symbol conflict is the same for static libraries
> as for dynamic libraries (i.e. in practice zero, as long as a consistent
> naming scheme is used for exported functions).
>
> 2) Remove the work-arounds to exclude duplicated symbols.
>
> 3) Fix some code in hotspot and the JDK libraries that did not work properly
> with a static java launcher.
>
> The latter fixes are copied from or inspired by the work done by @jianglizhou
> and her team as part of the Project Leyden [Hermetic
> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).
Some open questions:
* Do `os::lookup_function` need to be implemented on Windows too, for symmetry,
even if it is only used on Unix platforms?
* Many of the changes in Hotspot boils down to `os::dll_load` doing the wrong
thing when running with a static build. Perhaps we should provide a better
function that knows how to find and load a symbol for both static and dynamic
builds, and use that instead of making a lot of tests for static/dynamic on
each location we need to look up a symbol from some other JDK library.
* I managed to replace most of the #ifdef STATIC_BUILD with runtime checks.
There are some places remaining though. Apart from the #ifdefs needed for
JNI/JVMTI, which will need spec changes to address, there are code in
java_md_macosx.m, jio.c and awt_Mlib.c that I did not manage to turn into
runtime checks. They will need some more thorough work than just changing an
`#ifdef` to an `if () {`.
* And of course, the code in the build system to share all .o files except the
two linktype files is still under development...
I moved this away from Draft state, since I think it needs some visibility,
especially since it touches several different parts of the code base, and such
reviews tend to take time.
I think the code here is good and basically okay to integrate. This patch will
not on it's own solve the entire problem of building a proper static launcher,
but it takes several important steps along the way. I think the changes here
are reasonable to integrate into mainline at this point.
-
PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2140743300
PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2168635393
RFR: 8333268: Fixes for static build
This patch contains a set of changes to improve static builds. They will pave the way for implementing a full static-only java launcher. The changes here will: 1) Make sure non-exported symbols are made local in the static libraries. This means that the risk of symbol conflict is the same for static libraries as for dynamic libraries (i.e. in practice zero, as long as a consistent naming scheme is used for exported functions). 2) Remove the work-arounds to exclude duplicated symbols. 3) Fix some code in hotspot and the JDK libraries that did not work properly with a static java launcher. The latter fixes are copied from or inspired by the work done by @jianglizhou and her team as part of the Project Leyden [Hermetic Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). - Commit messages: - Merge branch 'master' into static-linking-progress - Move the exported JVM_IsStaticallyLinked to a better location - Use runtime lookup of static vs dynamic instead of #ifdef STATIC_BUILD - Copy fix for init_system_properties_values on linux - Make sure we do not try to build static libraries on Windows - 8333268: Fixes for static build Changes: https://git.openjdk.org/jdk/pull/19478/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19478&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333268 Stats: 440 lines in 28 files changed: 203 ins; 74 del; 163 mod Patch: https://git.openjdk.org/jdk/pull/19478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19478/head:pull/19478 PR: https://git.openjdk.org/jdk/pull/19478
Re: RFR: 8333867: SHA3 performance can be improved [v3]
On Fri, 14 Jun 2024 10:39:45 GMT, Ferenc Rakoczi wrote: >> This PR removes some unnecessary conversions between byte arrays and long >> arrays during SHA3 digest computations. > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accept more review suggestions Looks good - Marked as reviewed by valeriep (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19632#pullrequestreview-2118989182
Re: RFR: 8333867: SHA3 performance can be improved [v3]
On Fri, 14 Jun 2024 10:39:45 GMT, Ferenc Rakoczi wrote:
>> This PR removes some unnecessary conversions between byte arrays and long
>> arrays during SHA3 digest computations.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Accept more review suggestions
src/java.base/share/classes/sun/security/provider/SHA3.java line 137:
> 135: asLittleEndian.set(out, ofs, state[numLongs - 1]);
> 136: } else {
> 137: asLittleEndian.set(byteState, 0, state[numLongs - 1]);
So, is the performance better placing the `byteState` declaration up there at
line 111 instead of here? It's only used in this block...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1640180265
Re: RFR: 8333867: SHA3 performance can be improved [v2]
On Fri, 14 Jun 2024 09:47:31 GMT, Ferenc Rakoczi wrote: >> src/java.base/share/classes/sun/security/provider/SHA3.java line 73: >> >>> 71: // The following array is allocated to size WIDTH bytes, but we only >>> 72: // ever use the first blockSize bytes it (for bytes <-> long >>> conversions) >>> 73: private byte[] byteState = new byte[WIDTH]; >> >> Since we are storing the state in longs now, this "byte <-> long" conversion >> can be made through a local variable, right? Is there a reason for having >> this `byteState` field with size WIDTH bytes? > > This is interesting: if I use WIDTH (or in blockSize) long arrays in the > local level, the performance drops a few per cents. Even more when I only > declare the local array in the block in which it is used. However, since we > really only need 8 bytes, if I allocate that at the beginning of the > function, I don't see that performance drop. So I rewrote the output loop in > the function and got rid of the class level declaration. Interesting... Good that we go with the local 8-byte approach, it improves the readability also. - PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1640166504
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null [v10]
> 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: copyright 2024 - Changes: - all: https://git.openjdk.org/jdk/pull/19626/files - new: https://git.openjdk.org/jdk/pull/19626/files/b5ad8e70..1a012f1c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19626&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19626&range=08-09 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19626.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19626/head:pull/19626 PR: https://git.openjdk.org/jdk/pull/19626
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v2]
> Currently, javap crashes for class files that have set non-zero values for > undefined access flag bits, as > `java.lang.reflect.AccessFlag.maskToAccessFlag` and > `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though > asking for these bits to be set to 0, requires VM to proceed and ignore these > bits. javap should emulate the VM behavior and proceed rendering, ignoring > these undefined bits. > > In addition, a few bytecode generation utilities in the JDK set unused bits, > such as in > `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and > `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those > can be updated in a later cleanup. Chen Liang has updated the pull request incrementally with two additional commits since the last revision: - I am a dumbass - Retain strict flag for Method, even though it's obsolete - Changes: - all: https://git.openjdk.org/jdk/pull/19708/files - new: https://git.openjdk.org/jdk/pull/19708/files/1cf092f4..84506788 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19708&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19708&range=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19708.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19708/head:pull/19708 PR: https://git.openjdk.org/jdk/pull/19708
Integrated: 8330702: Update failure handler to don't generate Error message if cores actions are empty
On Thu, 30 May 2024 02:28:56 GMT, Leonid Mesnik wrote: > The message is generated if cores (or any other tools) section doesn't exist > or is empty. However, there is no any tool for cores processing now defined. > So ERROR message is generating, confusing users. > The fix is to don't print error for empty toolset which is the valid case. > The message is still generate is tool is not defined to get error message in > the case of miswriting. This pull request has now been integrated. Changeset: 548e95a6 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/548e95a689d63e97ddbdfe7dd7df3a2e3377046c Stats: 9 lines in 2 files changed: 5 ins; 0 del; 4 mod 8330702: Update failure handler to don't generate Error message if cores actions are empty Reviewed-by: sspitsyn - PR: https://git.openjdk.org/jdk/pull/19470
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:51:53 GMT, Weijun Wang wrote: >> It needs to recognise and throw RuntimeException so that a SecurityException >> isn't wrapped in a PrivilegedActionException (which gets caught by those >> blocks of code which call extractException(pe) and look at what Exception >> it contains). >> >> (test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java >> tests this) >> >> I can add a check to make sure PrivilgedActionException doesn't get wrapped. >> Daniel I think mentioned this also. > > I still don't understand how a SecurityException could be wrapped into a PAE > without these lines. I misunderstood the comment, yes I think I can remove this block now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639983304
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Unnecessary catches to remove - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/d5f68b02..13d9202f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=11-12 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:00:58 GMT, Kevin Walls wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
>> line 1461:
>>
>>> 1459: throw rte;
>>> 1460: } else {
>>> 1461: throw new PrivilegedActionException(e);
>>
>> The 4 lines above seems unnecessary now. Plus, do not wrap
>> `PrivilegedActionException` inside `PrivilegedActionException`.
>
> It needs to recognise and throw RuntimeException so that a SecurityException
> isn't wrapped in a PrivilegedActionException (which gets caught by those
> blocks of code which call extractException(pe) and look at what Exception it
> contains).
>
> (test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java
> tests this)
>
> I can add a check to make sure PrivilgedActionException doesn't get wrapped.
> Daniel I think mentioned this also.
I still don't understand how a SecurityException could be wrapped into a PAE
without these lines.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639950681
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:41:20 GMT, Kevin Walls wrote: > Does noPermissionsACC add anything? I don't know. My principal for this code change is that nothing is changed for the SM-is-allowed case. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2168203868
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:03:07 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Separate SM allowed and not allowed cases
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1461:
>
>> 1459: throw rte;
>> 1460: } else {
>> 1461: throw new PrivilegedActionException(e);
>
> The 4 lines above seems unnecessary now. Plus, do not wrap
> `PrivilegedActionException` inside `PrivilegedActionException`.
It needs to recognise and throw RuntimeException so that a SecurityException
isn't wrapped in a PrivilegedActionException (which gets caught by those blocks
of code which call extractException(pe) and look at what Exception it
contains).
(test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java
tests this)
I can add a check to make sure PrivilgedActionException doesn't get wrapped.
Daniel I think mentioned this also.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639876260
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 20:52:51 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>>Undo test policy updates
>
> src/java.management/share/classes/javax/management/monitor/Monitor.java line
> 1543:
>
>> 1541: // No SecurityManager:
>> 1542: Subject.doAs(s, action); // s is permitted to be null
>> 1543: } else {
>
> Even though ac should never be null, I would keep the original check for ac
> == null inside the SM code path to be safe and consistent with the original
> code, and use only call doAs if allowSM is false, so like:
>
>
> if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
> // No SecurityManager:
> Subject.doAs(s, action); // s is permitted to be null
> } else {
> if (ac == null) {
> throw new SecurityException("AccessControlContext cannot be null");
> }
> // ACC means SM is permitted.
> AccessController.doPrivileged(action, ac);
> }
ok done!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639833457
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v12]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Monitor update - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/242af991..d5f68b02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=10-11 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:44:17 GMT, Kevin Walls wrote:
>> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
>> line 353:
>>
>>> 351: } else {
>>> 352: return Subject.getSubject(AccessController.getContext());
>>> 353: }
>>
>> `Subject.current()` should work for both cases. See the impl of it.
>
> It will work to get the Subject yes.
> Do I not need the SM-enabled case, in case there some complex ACC in place?
> (combiner)
OK if it was actually getting specifically the ACC to use it maybe, but this
just needs to resolve a Subject.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639806380
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v11]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: ServerNotifForwarder simplification - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/09c2e38f..242af991 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=09-10 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19052#pullrequestreview-2118319536
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 14 Jun 2024 10:52:40 GMT, Magnus Ihse Bursie wrote: >> Ah, I had not realized that there was more than 1 newline. GitHub's UI >> confused me here, so we're good to go > > GitHub's UI assumes the final line has an line break. If it is missing, it > displays a red 🚫 at the end of the last line. If there is an empty line > showing up in the UI, then it is an additional empty line. In this case, GitHub is actually not showing up extra empty lines, that they appear the same as the single line break in the end of the file. - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1639783594
Integrated: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher
On Thu, 6 Jun 2024 11:53:10 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to remove the > `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? > > This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that > JBS issue, in a recent PR discussion, it was suggested > https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this > macro should be removed and the failure of a JNI specified operation (the > ones for which this macro is being used) should be determined based on a > `NULL` value returned from that function. The commit in this PR removes this > macros and updates the call sites to do a `NULL` check. > > Given the nature of this change, no new tests have been added. tier1, tier2 > and tier3 testing passed successfully with these changes. This pull request has now been integrated. Changeset: efab48c0 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/efab48c06554476eae7a7bd946dee033d16a9c38 Stats: 68 lines in 1 file changed: 37 ins; 9 del; 22 mod 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/19576
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:04:06 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Separate SM allowed and not allowed cases
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 353:
>
>> 351: } else {
>> 352: return Subject.getSubject(AccessController.getContext());
>> 353: }
>
> `Subject.current()` should work for both cases. See the impl of it.
ok!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639766973
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:09:46 GMT, Weijun Wang wrote: > I don't quite understand why there is no more `noPermissionsACC` in > `Monotor.java`. This looks like the only behavior change when SM is allowed. > The other source change looks fine to me. Does noPermissionsACC add anything? Maybe I'm rushing the removal of ACCs where I can. Start sets acc to the current context (when SM permitted, use Subject otherwise) Set acc to noPermissionsACC when stop is called (or forget the Subject). acc is accessed within the run method. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2167949489
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Thu, 13 Jun 2024 20:54:25 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Separate SM allowed and not allowed cases
I don't quite understand why there is no more `noPermissionsACC` in
`Monotor.java`. This looks like the only behavior change when SM is allowed.
The other source change looks fine to me.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1461:
> 1459: throw rte;
> 1460: } else {
> 1461: throw new PrivilegedActionException(e);
The 4 lines above seems unnecessary now. Plus, do not wrap
`PrivilegedActionException` inside `PrivilegedActionException`.
src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
line 353:
> 351: } else {
> 352: return Subject.getSubject(AccessController.getContext());
> 353: }
`Subject.current()` should work for both cases. See the impl of it.
-
PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2118220033
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639720192
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639721194
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 13:34:45 GMT, Julian Waters wrote: >> I find the extra trailing newlines through below shell command: >> >> for i in `find . -iname "Makefile*" | sed "/./build/d"` ; do tail -n 2 $i | >> grep -c "^$" | grep -q "^1$" ; if [[ 0 -eq $? ]] ; then echo $i ; fi ; done >> >> >> There are only two files has been found: >> >> ./test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile >> ./test/jdk/java/rmi/reliability/benchmark/bench/Makefile > > Ah, I had not realized that there was more than 1 newline. GitHub's UI > confused me here, so we're good to go GitHub's UI assumes the final line has an line break. If it is missing, it displays a red 🚫 at the end of the last line. If there is an empty line showing up in the UI, then it is an additional empty line. - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1639649265
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Looks good to me. I assume that you have run an extensive set of tests to verify that this does not break, even in higher tiers? - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19052#pullrequestreview-2118094672
Re: RFR: 8333867: SHA3 performance can be improved [v3]
> This PR removes some unnecessary conversions between byte arrays and long > arrays during SHA3 digest computations. Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision: Accept more review suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/19632/files - new: https://git.openjdk.org/jdk/pull/19632/files/0a9f9624..4fcdd09f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19632&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19632&range=01-02 Stats: 21 lines in 1 file changed: 2 ins; 8 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/19632.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19632/head:pull/19632 PR: https://git.openjdk.org/jdk/pull/19632
Re: RFR: 8333962: Obsolete OldSize [v2]
> Obsolete OldSize and related code. An internal variable `OldSize` is kept to > capture the capacity of old-gen size. Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: obsolete-old-size - Changes: https://git.openjdk.org/jdk/pull/19647/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19647&range=01 Stats: 192 lines in 15 files changed: 7 ins; 168 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/19647.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19647/head:pull/19647 PR: https://git.openjdk.org/jdk/pull/19647
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 21:03:17 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>>Undo test policy updates
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1304:
>
>> 1302: // No ACC, therefore no SM. May have a Subject:
>> 1303: if (subject != null) {
>> 1304: return Subject.doAs(subject, action);
>
> Is it ever possible for acc to be `null` and `subject` not `null` and an SM
> to be enabled? Doesn't look like it, but if it ever could be, then the call
> above to `Subject.doAs` would trigger a permission check for an
> `AuthPermission("doAs")` permission.
>
> I think following Weijun's advice above is cleaner and safer, so you do one
> or the other depending on the allowSM setting, and not whether certain
> variables are null or not.
Right, the only possible assignment to acc in this file is if we were given a
Subject, and SM is permitted.
In future there will be a Subject, which can be null. While we handle SM, we
still use the ACC if RMIConnectionImpl was created with a Subject.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639610678
Re: RFR: 8333867: SHA3 performance can be improved [v2]
On Thu, 13 Jun 2024 20:25:22 GMT, Valerie Peng wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix clone(), accept review suggestions. > > src/java.base/share/classes/sun/security/provider/SHA3.java line 73: > >> 71: // The following array is allocated to size WIDTH bytes, but we only >> 72: // ever use the first blockSize bytes it (for bytes <-> long >> conversions) >> 73: private byte[] byteState = new byte[WIDTH]; > > Since we are storing the state in longs now, this "byte <-> long" conversion > can be made through a local variable, right? Is there a reason for having > this `byteState` field with size WIDTH bytes? This is interesting: if I use WIDTH (or in blockSize) long arrays in the local level, the performance drops a few per cents. Even more when I only declare the local array in the block in which it is used. However, since we really only need 8 bytes, if I allocate that at the beginning of the function, I don't see that performance drop. So I rewrote the output loop in the function and got rid of the class level declaration. > src/java.base/share/classes/sun/security/provider/SHA3.java line 121: > >> 119: } >> 120: implCompress(buffer, 0); >> 121: int availableBytes = buffer.length; > > change to `blockSize` as in `implCompress0(...)`? Yes, thanks, I wanted to, just forgot. - PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1639572500 PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1639572413
Re: RFR: 8333867: SHA3 performance can be improved [v2]
On Fri, 14 Jun 2024 05:56:05 GMT, Andrey Turbanov wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix clone(), accept review suggestions.
>
> src/java.base/share/classes/sun/security/provider/SHA3.java line 152:
>
>> 150: */
>> 151: void implReset() {
>> 152: Arrays.fill(state,0L);
>
> Suggestion:
>
> Arrays.fill(state, 0L);
Thanks! Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19632#discussion_r1639572679
RFR: 8331552: Update to use jtreg 7.4
Please review the change to update to using `jtreg` **7.4**. The primary change is to the `jib-profiles.js` file, which specifies the version of jtreg to use, for those systems that rely on this file. In addition, the `requiredVersion` has been updated in the various `TEST.ROOT` files. Testing: _tier1-tier5 pending..._ - Commit messages: - 8331552: Update to use jtreg 7.4 Changes: https://git.openjdk.org/jdk/pull/19052/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19052&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331552 Stats: 12 lines in 8 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19052/head:pull/19052 PR: https://git.openjdk.org/jdk/pull/19052
Integrated: 8332400: isspace argument should be a valid unsigned char
On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga wrote: > ### Summary > This change ensures we don't get undefined behavior when > calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). > `isspace` accepts an `int` argument that "the application shall ensure is a > character representable as an unsigned char or equal to the value of the > macro EOF.". > > Previously, there was no checking of the values passed to `isspace`. I've > replaced direct calls with a new wrapper `os::is_space` that performs a range > check and prevents the possibility of undefined behavior from happening. For > instances outside of Hotspot, I've added casts to `unsigned char`. > > **Testing** > - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check > `os::is_space` is working correctly. > - tier1 This pull request has now been integrated. Changeset: cc64aeac Author:Robert Toyonaga Committer: Thomas Stuefe URL: https://git.openjdk.org/jdk/commit/cc64aeac47917f20a6d70e9796f0de9aa165ce62 Stats: 17 lines in 8 files changed: 0 ins; 0 del; 17 mod 8332400: isspace argument should be a valid unsigned char Reviewed-by: dholmes, amitkumar, stuefe, jwaters - PR: https://git.openjdk.org/jdk/pull/19567
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Fri, 14 Jun 2024 06:49:34 GMT, Alan Bateman wrote: > Yes, on my list. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2167591811
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null [v9]
On Fri, 14 Jun 2024 01:17:29 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: > > optimization for x64 The performance regression issue under x64 has been resolved. The following code will affect the running performance under x64: * x64 slow version int count = this.count; ensureCapacityInternal(count + 4); * x64 faster version, WebRev 08: [Full](https://webrevs.openjdk.org/?repo=jdk&pr=19626&range=08) - [Incremental](https://webrevs.openjdk.org/?repo=jdk&pr=19626&range=07-08) ([b5ad8e70](https://git.openjdk.org/jdk/pull/19626/files/b5ad8e70928c547d134d0e4a532441cad9a7e4a2)) ensureCapacityInternal(count + 4); int count = this.count; I don't know why, but this solved the problem. The performance numbers for various platforms are as follows ## aliyun ecs.c8a * CPU AMD EPYCTM Genoa * Platform x64 -Benchmark Mode Cnt Score Error Units #master (a6fc2f8) -StringBuilders.appendWithBool8Latin1 avgt 15 7.270 ± 0.013 ns/op -StringBuilders.appendWithBool8Utf16 avgt 15 10.860 ± 0.044 ns/op -StringBuilders.appendWithNull8Latin1 avgt 15 5.490 ± 0.007 ns/op -StringBuilders.appendWithNull8Utf16 avgt 15 26.203 ± 0.294 ns/op +Benchmark Mode Cnt Score Error Units # Web 08 (b5ad8e70) +StringBuilders.appendWithBool8Latin1 avgt 15 6.035 ± 0.062 ns/op +20% +StringBuilders.appendWithBool8Utf16 avgt 15 10.072 ± 0.032 ns/op +7.82% +StringBuilders.appendWithNull8Latin1 avgt 15 5.491 ± 0.021 ns/op -0.01$ +StringBuilders.appendWithNull8Utf16 avgt 15 7.701 ± 0.036 ns/op +240.25% ## aliyun ecs.c8i * CPU Intel® Xeon® Emerald * Platform x64 -Benchmark Mode Cnt Score Error Units #master (a6fc2f8) -StringBuilders.appendWithBool8Latin1 avgt 15 7.466 ± 0.012 ns/op -StringBuilders.appendWithBool8Utf16 avgt 15 15.220 ± 0.005 ns/op -StringBuilders.appendWithNull8Latin1 avgt 15 22.084 ± 0.331 ns/op -StringBuilders.appendWithNull8Utf16 avgt 15 25.717 ± 0.106 ns/op +Benchmark Mode Cnt Score Error Units # Web 08 (b5ad8e70) +StringBuilders.appendWithBool8Latin1 avgt 15 6.706 ± 0.011 ns/op +11.33% +StringBuilders.appendWithBool8Utf16 avgt 15 11.610 ± 0.140 ns/op +31.09% +StringBuilders.appendWithNull8Latin1 avgt 15 19.345 ± 0.050 ns/op +14.15% +StringBuilders.appendWithNull8Utf16 avgt 15 22.765 ± 0.045 ns/op +12.96% ## MacBook M1 Max * CPU Apple M1 Max * Platform aarch64 -Benchmark Mode Cnt Score Error Units #master (a6fc2f8) -StringBuilders.appendWithBool8Latin1 avgt 15 7.647 ? 0.014 ns/op -StringBuilders.appendWithBool8Utf16 avgt 15 9.620 ? 0.022 ns/op -StringBuilders.appendWithNull8Latin1 avgt 15 7.208 ? 0.127 ns/op -StringBuilders.appendWithNull8Utf16 avgt 15 9.455 ? 0.167 ns/op +Benchmark Mode Cnt Score Error Units # Web 08 (b5ad8e70) +StringBuilders.appendWithBool8Latin1 avgt 15 5.932 ? 0.026 ns/op +28.91% +StringBuilders.appendWithBool8Utf16 avgt 15 7.432 ? 0.015 ns/op +29.44$ +StringBuilders.appendWithNull8Latin1 avgt 15 5.461 ? 0.015 ns/op +31.99% +StringBuilders.appendWithNull8Utf16 avgt 15 6.966 ? 0.015 ns/op +35.73% ## aliyun ecs.c8y * CPU Yitian710 aarch64 * Platform aarch64 -Benchmark Mode Cnt Score Error Units -StringBuilders.appendWithBool8Latin1 avgt 15 12.127 ± 1.241 ns/op -StringBuilders.appendWithBool8Utf16 avgt 15 20.954 ± 1.668 ns/op -StringBuilders.appendWithNull8Latin1 avgt 15 10.838 ± 0.244 ns/op -StringBuilders.appendWithNull8Utf16 avgt 15 14.391 ± 0.063 ns/op +Benchmark Mode Cnt Score Error Units +StringBuilders.appendWithBool8Latin1 avgt 15 9.299 ± 0.079 ns/op +30.41% +StringBuilders.appendWithBool8Utf16 avgt 15 11.688 ± 0.004 ns/op +79.27% +StringBuilders.appendWithNull8Latin1 avgt 15 8.516 ± 0.258 ns/op +27.26% +StringBuilders.appendWithNull8Utf16 avgt 15 11.020 ± 0.004 ns/op +30.58% ## aws.c5 * Platform aarch64 -Benchmark Mode Cnt Score Error Units -StringBuilders.appendWithBool8Latin1 avgt 15 10.278 ± 0.071 ns/op -StringBuilders.appendWithBool8Utf16 avgt 15 24.355 ± 0.418 ns/op -StringBuilders.appendWithNull8Latin1 avgt 15 33.104 ± 0.589 ns/op -StringBuilders.appendWithNull8Utf16 avgt 15 35.762 ± 7.580 ns/op +Benchmark Mode Cnt Score Error Units +StringBuilders.appendWithBool8Latin1 avgt
