-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8358880: Performance of parsing with DecimalFormat can be improved #25644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
8bf1f61
9644568
bcac896
e13dea8
a6978bd
a85ddd8
da9e4ae
6953dcf
6a07287
c87a3de
b7faa3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,13 +169,7 @@ public final double getDouble() { | |
if (count == 0) { | ||
return 0.0; | ||
} | ||
|
||
return Double.parseDouble(getStringBuilder() | ||
.append('.') | ||
.append(digits, 0, count) | ||
.append('E') | ||
.append(decimalAt) | ||
.toString()); | ||
return FloatingDecimal.parseDoubleDigits(decimalAt, digits, count); | ||
} | ||
|
||
/** | ||
|
@@ -190,17 +184,18 @@ public final long getLong() { | |
return 0; | ||
} | ||
|
||
// We have to check for this, because this is the one NEGATIVE value | ||
// Parse as unsigned to handle Long.MIN_VALUE, which is the one NEGATIVE value | ||
// we represent. If we tried to just pass the digits off to parseLong, | ||
// we'd get a parse failure. | ||
if (isLongMIN_VALUE()) { | ||
return Long.MIN_VALUE; | ||
long v = Long.parseUnsignedLong(new String(digits, 0, count)); | ||
if (v < 0) { | ||
if (Long.compareUnsigned(v, Long.MIN_VALUE) == 0) { | ||
return Long.MIN_VALUE; | ||
} | ||
throw new NumberFormatException("Unexpected negative value"); | ||
} | ||
|
||
StringBuilder temp = getStringBuilder(); | ||
temp.append(digits, 0, count); | ||
temp.append("0".repeat(Math.max(0, decimalAt - count))); | ||
return Long.parseLong(temp.toString()); | ||
long pow10 = Math.powExact(10L, Math.max(0, decimalAt - count)); | ||
return Math.multiplyExact(v, pow10); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two methods throw ArithmeticException. This needs to be rethrown as NumberFormatException. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is a little odd. The parse methods that call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will leave this question to I18N reviewers, who are ultimately in charge of DigitList. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing implementation does not throw There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I'm not seeing where the original could throw ParseException. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if I was unclear. I mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I've added a commit to follow @liach's suggestion to at least handle the ArithmeticException so as to not introduce new exceptions into the mix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, sounds reasonable. |
||
} | ||
|
||
/** | ||
|
@@ -726,35 +721,18 @@ public Object clone() { | |
System.arraycopy(digits, 0, newDigits, 0, digits.length); | ||
other.digits = newDigits; | ||
|
||
// data and tempBuilder do not need to be copied because they do | ||
// not carry significant information. They will be recreated on demand. | ||
// Setting them to null is needed to avoid sharing across clones. | ||
// Data does not need to be copied because it does | ||
// not carry significant information. It will be recreated on demand. | ||
// Setting it to null is needed to avoid sharing across clones. | ||
other.data = null; | ||
other.tempBuilder = null; | ||
|
||
return other; | ||
} catch (CloneNotSupportedException e) { | ||
throw new InternalError(e); | ||
} | ||
} | ||
|
||
/** | ||
* Returns true if this DigitList represents Long.MIN_VALUE; | ||
* false, otherwise. This is required so that getLong() works. | ||
*/ | ||
private boolean isLongMIN_VALUE() { | ||
if (decimalAt != count || count != MAX_COUNT) { | ||
return false; | ||
} | ||
|
||
for (int i = 0; i < count; ++i) { | ||
if (digits[i] != LONG_MIN_REP[i]) return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
private static final int parseInt(char[] str, int offset, int strLen) { | ||
private static int parseInt(char[] str, int offset, int strLen) { | ||
char c; | ||
boolean positive = true; | ||
if ((c = str[offset]) == '-') { | ||
|
@@ -787,17 +765,6 @@ public String toString() { | |
return "0." + new String(digits, 0, count) + "x10^" + decimalAt; | ||
} | ||
|
||
private StringBuilder tempBuilder; | ||
|
||
private StringBuilder getStringBuilder() { | ||
if (tempBuilder == null) { | ||
tempBuilder = new StringBuilder(MAX_COUNT); | ||
} else { | ||
tempBuilder.setLength(0); | ||
} | ||
return tempBuilder; | ||
} | ||
|
||
private void extendDigits(int len) { | ||
if (len > digits.length) { | ||
digits = new char[len]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright (c) 1996, 2024, Oracle and/or its affiliates. All rights reserved. | ||
* Copyright (c) 1996, 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
|
@@ -122,6 +122,19 @@ public static float parseFloat(String s) throws NumberFormatException { | |
return readJavaFormatString(s, BINARY_32_IX).floatValue(); | ||
} | ||
|
||
/** | ||
* Converts a sequence of digits ('0'-'9') as well as an exponent to a positive | ||
* double value | ||
* | ||
* @param decExp The decimal exponent of the value to generate | ||
* @param digits The digits of the significand. | ||
* @param length Number of digits to use | ||
* @return The double-precision value of the conversion | ||
*/ | ||
public static double parseDoubleDigits(int decExp, char[] digits, int length) throws NumberFormatException { | ||
j3graham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return readDoubleDigits(decExp, digits, length).doubleValue(); | ||
} | ||
|
||
/** | ||
* A converter which can process single or double precision floating point | ||
* values into an ASCII <code>String</code> representation. | ||
|
@@ -1824,6 +1837,17 @@ private static BinaryToASCIIConverter getBinaryToASCIIConverter(float f) { | |
return buf; | ||
} | ||
|
||
static ASCIIToBinaryConverter readDoubleDigits(int decExp, char[] digits, int length) { | ||
if (decExp < MIN_DECIMAL_EXPONENT) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this check needed? I think
And if this explicit check is a shortcut, I don't think we would need one for an edge case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately some check is required (a test fails), but I now realize what I had was wrong. The issue is that on line 1084 (https://github.com/openjdk/jdk/pull/25644/files#diff-79e6fd549b5ec5e7f49658581beddcb07fcbb0c09ae8e1117c385b66514da6d2R1084)) exp can overflow and become positive again. I've updated the check to avoid the overflow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah got it, I see your point. We would have goten underflow in Is there a specific example you have that requires the switch to the newer check? Adding a comment along those lines might be helpful. Actually, I thought DigitList caps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a specific example, so I've reverted to my original check. I'm a bit unsettled by the check for an extreme value later in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, the original check you had is easier to understand what is happening without further context, so I prefer your switch back. I think we are fine from (negative) "extreme values" in Also, should the check be against |
||
return buildZero(BINARY_64_IX, 1); | ||
} | ||
byte[] buf = new byte[length]; | ||
for (int i = 0; i < length; i++) { | ||
buf[i] = (byte) digits[i]; | ||
} | ||
return new ASCIIToBinaryBuffer(false, decExp, buf, length); | ||
j3graham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* The input must match the {@link Double#valueOf(String) rules described here}, | ||
* about leading and trailing whitespaces, and the grammar. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/* | ||
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
package org.openjdk.bench.java.text; | ||
|
||
import java.text.NumberFormat; | ||
import java.text.DecimalFormat; | ||
import java.text.ParseException; | ||
import java.util.Locale; | ||
j3graham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import java.util.concurrent.TimeUnit; | ||
|
||
import org.openjdk.jmh.annotations.Benchmark; | ||
import org.openjdk.jmh.annotations.BenchmarkMode; | ||
import org.openjdk.jmh.annotations.Fork; | ||
import org.openjdk.jmh.annotations.Measurement; | ||
import org.openjdk.jmh.annotations.Mode; | ||
import org.openjdk.jmh.annotations.OperationsPerInvocation; | ||
import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
import org.openjdk.jmh.annotations.Scope; | ||
import org.openjdk.jmh.annotations.Setup; | ||
import org.openjdk.jmh.annotations.State; | ||
import org.openjdk.jmh.annotations.Warmup; | ||
import org.openjdk.jmh.infra.Blackhole; | ||
import org.openjdk.jmh.runner.Runner; | ||
import org.openjdk.jmh.runner.options.Options; | ||
import org.openjdk.jmh.runner.options.OptionsBuilder; | ||
|
||
@BenchmarkMode(Mode.Throughput) | ||
@OutputTimeUnit(TimeUnit.MILLISECONDS) | ||
@Warmup(iterations = 5, time = 1) | ||
@Measurement(iterations = 5, time = 1) | ||
@Fork(3) | ||
@State(Scope.Benchmark) | ||
public class DecimalFormatParseBench { | ||
|
||
public String[] valuesLong; | ||
public String[] valuesDouble; | ||
|
||
@Setup | ||
public void setup() { | ||
valuesLong = new String[]{ | ||
"123", "149", "180", "170000000000000000", "0", "-149", "-15000", "99999123", "1494", "1495", "1030", "25996", "-25996" | ||
}; | ||
|
||
valuesDouble = new String[]{ | ||
"1.23", "1.49", "1.80", "17000000000000000.1", "0.01", "-1.49", "-1.50", "9999.9123", "1.494", "1.495", "1.03", "25.996", "-25.996" | ||
}; | ||
} | ||
|
||
private NumberFormat dnf = new DecimalFormat(); | ||
|
||
@Benchmark | ||
@OperationsPerInvocation(13) | ||
public void testParseLongs(final Blackhole blackhole) throws ParseException { | ||
for (String value : valuesLong) { | ||
blackhole.consume(this.dnf.parse(value)); | ||
} | ||
} | ||
|
||
@Benchmark | ||
@OperationsPerInvocation(13) | ||
public void testParseDoubles(final Blackhole blackhole) throws ParseException { | ||
for (String value : valuesDouble) { | ||
blackhole.consume(this.dnf.parse(value)); | ||
} | ||
} | ||
public static void main(String... args) throws Exception { | ||
Options opts = new OptionsBuilder().include(DefFormatterBench.class.getSimpleName()).shouldDoGC(true).build(); | ||
new Runner(opts).run(); | ||
} | ||
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.