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: 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: 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: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
On Tue, 25 Jun 2024 14:47:45 GMT, Raffaello Giulietti wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Utf16 case remove `append first utf16 char` > > 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? - PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1653210427
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
On Mon, 17 Jun 2024 04:58:41 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:
>
> Utf16 case remove `append first utf16 char`
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 43:
> 41: public final class DoubleToDecimal extends ToDecimal {
> 42: /**
> 43: * Use LATIN1 encoding to process the input byte[] str
Suggestion:
* Use LATIN1 encoding to process the in-out byte[] str
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 49:
> 47:
> 48: /**
> 49: * Use UTF16 encoding to process the input byte[] str
Suggestion:
* Use UTF16 encoding to process the in-out byte[] str
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 175:
> 173: */
> 174: public int putDecimal(byte[] str, int index, double v) {
> 175: assert index >= 0 && index + MAX_CHARS <= length(str) : "Trusted
> caller missed bounds check";
Suggestion:
assert 0 <= index && index <= length(str) - MAX_CHARS : "Trusted caller
missed bounds check";
This avoids a potential overflow.
But if you want to enforce the check, you should probably avoid using `assert`
since assertions might be disabled.
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;
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 452:
> 450: if (l != 0) {
> 451: put8Digits(str, index, l);
> 452: index += 8;
As discussed in `ToDecimal`, all `put...` methods should return the updated
`index`. Then this code can be rewritten as
Suggestion:
index = put8Digits(str, index, l);
so that the caller doesn't need to know how many characters were added.
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 460:
> 458: putChar(str, index++, 'E');
> 459: if (e < 0) {
> 460: putChar(str, index++, '-');
By
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
On Mon, 17 Jun 2024 04:58:41 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:
>
> Utf16 case remove `append first utf16 char`
I would request a review from @rgiulietti, author of the original code.
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 154:
> 152: int length = s.length();
> 153: if (coder == LATIN1) {
> 154: for (int i = 0; i < length; ++i) {
Can we use `s.getBytes(0, length, str, index)` and add
`@SuppressWarnings("deprecation")` for performance?
-
PR Review: https://git.openjdk.org/jdk/pull/19730#pullrequestreview-2137225522
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1651921455
Re: RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v13]
> 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:
Utf16 case remove `append first utf16 char`
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19730/files
- new: https://git.openjdk.org/jdk/pull/19730/files/05d5b5cf..b8e80428
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=12
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19730&range=11-12
Stats: 2 lines in 1 file changed: 0 ins; 2 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
