Skip to content

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 14 additions & 47 deletions src/java.base/share/classes/java/text/DigitList.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two methods throw ArithmeticException. This needs to be rethrown as NumberFormatException.

Copy link
Contributor Author

@j3graham j3graham Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a little odd. The parse methods that call getLong are not supposed to throw NumberFormatException either. So wherever getLong is called, it must be preceded by a check to fitsIntoLong, which should avoid any exceptions here. That said, rethrowing as NFE would avoid new surprises. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing implementation does not throw NumberFormatException/ArithmeticException, but ParseException if parsing is failing. I would expect the same here.

Copy link
Contributor Author

@j3graham j3graham Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not seeing where the original could throw ParseException.

Copy link
Member

@naotoj naotoj Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I was unclear. I mean the parse() in the NumberFormat do not throw NumberFormatException/ArithmeticException, but ParseException, so if this piece of code need to throw something, it should be ParseException

Copy link
Contributor Author

@j3graham j3graham Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parse() methods where this code gets used in DecimalFormat unfortunately don't throw ParseException. The current calls to getLong always are guarded with a call to fitsIntoLong, which should avoid any exceptions actually being thrown here. So there is no parse failure as such - instead it tries to parse it as a double or a BigDecimal. If getLong were to be called without the guard, the exception would have come from Long.parseLong, which throws a NumberFormatException.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, sounds reasonable.

}

/**
Expand Down Expand Up @@ -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]) == '-') {
Expand Down Expand Up @@ -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];
Expand Down
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
Expand Down Expand Up @@ -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 {
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.
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check needed? I think ASCIIToBinaryConverter will return the proper zero value when doubleValue() is invoked.

  if (decExponent < MIN_DECIMAL_EXPONENT - 1) {
      return (isNegative) ? -0.0 : 0.0;

And if this explicit check is a shortcut, I don't think we would need one for an edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 ASCIIToBinaryConverter.doubleValue() for some extreme cases without a check.

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 decimalAt to Integer.MIN/MAX, so then the first check you had would have been fine. (Maybe I am missing something?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 doubleValue() comparing against MIN_DECIMAL_EXPONENT - 1

Copy link
Member

Choose a reason for hiding this comment

The 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 doubleValue() because of the check you have implemented in the first place. i.e. we avoid any potential underflow from int exp = decExponent - kDigits;. I think we do need a comment to accompany the check. (Why do we check? why not check the max exponent value?)

Also, should the check be against MIN_DECIMAL_EXPONENT - 1 for consistency with doubleValue()? (Functionally, I don't think it matters.)

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);
}

/**
* The input must match the {@link Double#valueOf(String) rules described here},
* about leading and trailing whitespaces, and the grammar.
Expand Down
5 changes: 0 additions & 5 deletions test/jdk/java/text/Format/DecimalFormat/CloneTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,6 @@ public void testClone() {
assertNotSame(data, valFromDigitList(dfClone, "data"));
}

Object tempBuilder = valFromDigitList(original, "tempBuilder");
if (tempBuilder != null) {
assertNotSame(data, valFromDigitList(dfClone, "data"));
}

assertEquals(digitListField.get(original), digitListField.get(dfClone));
} catch (ReflectiveOperationException e) {
throw new SkippedException("reflective access in white-box test failed", e);
Expand Down
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;
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();
}

}