Skip to content

Optimize parsing 19 digit longs #865

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 20, 2022
Merged

Optimize parsing 19 digit longs #865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6d50fc
Optimize parsing 19 digit longs
marschall Jul 24, 2022
d7501ee
Implement more number tests
marschall Dec 20, 2022
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
6 changes: 3 additions & 3 deletions src/main/java/com/fasterxml/jackson/core/base/ParserBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,6 @@ private void _parseSlowFloat(int expType) throws IOException

private void _parseSlowInt(int expType) throws IOException
{
final String numStr = _textBuffer.contentsAsString();
try {
int len = _intLength;
char[] buf = _textBuffer.getTextBuffer();
Expand All @@ -949,10 +948,10 @@ private void _parseSlowInt(int expType) throws IOException
}
// Some long cases still...
if (NumberInput.inLongRange(buf, offset, len, _numberNegative)) {
// Probably faster to construct a String, call parse, than to use BigInteger
_numberLong = NumberInput.parseLong(numStr);
_numberLong = NumberInput.parseLong19(buf, offset, _numberNegative);
_numTypesValid = NR_LONG;
} else {
String numStr = _textBuffer.contentsAsString();
// 16-Oct-2018, tatu: Need to catch "too big" early due to [jackson-core#488]
if ((expType == NR_INT) || (expType == NR_LONG)) {
_reportTooLongIntegral(expType, numStr);
Expand All @@ -970,6 +969,7 @@ private void _parseSlowInt(int expType) throws IOException
}
}
} catch (NumberFormatException nex) {
String numStr = _textBuffer.contentsAsString();
// Can this ever occur? Due to overflow, maybe?
_wrapError("Malformed numeric value ("+_longNumberDesc(numStr)+")", nex);
}
Expand Down
26 changes: 26 additions & 0 deletions src/main/java/com/fasterxml/jackson/core/io/NumberInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,32 @@ public static long parseLong(char[] ch, int off, int len)
long val = parseInt(ch, off, len1) * L_BILLION;
return val + (long) parseInt(ch, off+len1, 9);
}

/**
* Parses an unsigned long made up of exactly 19 digits.
Copy link
Member

@pjfanning pjfanning Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this looks wrong - the code seems to handle signed numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This the same approach that com.fasterxml.jackson.core.util.TextBuffer#contentsAsInt(boolean) uses, it calls com.fasterxml.jackson.core.io.NumberInput#parseInt(char[], int, int) that parses an optionally singed value unsigned by ignoring the sign and later flips the sign if it had a minus sign.

* <p>
* It is the callers responsibility to make sure the input is exactly 19 digits.
* and fits into a 64bit long by calling {@link #inLongRange(char[], int, int, boolean)}
* first.
* <p>
* Note that input String must NOT contain leading minus sign (even
* if {@code negative} is set to true).
*
* @param ch Buffer that contains integer value to decode
* @param off Offset of the first digit character in buffer
* @param negative Whether original number had a minus sign
* @return Decoded {@code long} value
*/
public static long parseLong19(char[] ch, int off, boolean negative)
{
// Note: caller must ensure length is [10, 18]
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this comment line which doesn't match this code?

long num = 0L;
for (int i = 0; i < 19; i++) {
char c = ch[off + i];
num = (num * 10) + (c - '0');
}
return negative ? -num : num;
}
Copy link
Member

@pjfanning pjfanning Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

can you provide a jmh test project to prove this is faster? you can extend https://github.com/pjfanning/jackson-number-parse-bench if you like - you obviously have the test case, just would be handy to be able to run it independently

Copy link
Member

Choose a reason for hiding this comment

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

if this is really such a performance drag, shouldn't there be an openjdk issue - or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this is really such a performance drag, shouldn't there be an openjdk issue - or something similar?

Outsiders can't create OpenJDK issues. And even if, the discussions alone would probably take years. The time investment is massive with no guaranteed outcome. To give you a concrete example openjdk/jdk#6275.

Even if everything would go smoothly we would still have to wait years until Jackson updates the minimum version to one that has the fixes. Jackson currently has a base of JDK 8 which was released 8 years ago. So we're looking at a best case of 10 years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you provide a jmh test project to prove this is faster? you can extend https://github.com/pjfanning/jackson-number-parse-bench if you like - you obviously have the test case, just would be handy to be able to run it independently

I saw there is also https://github.com/FasterXML/jackson-benchmarks let me know what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Whichever suits you. I have a vague recollection that jackson-benchmarks is configured to run the benchmarks for a long time. There was something about the project that made me go off and create my own where I could reconfigure the settings more readily.


/**
* Similar to {@link #parseInt(String)} but for {@code long} values.
Expand Down
12 changes: 11 additions & 1 deletion src/test/java/com/fasterxml/jackson/core/read/NumberParsingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,9 @@ public void testLongRange() throws Exception
for (int mode : ALL_MODES) {
long belowMinInt = -1L + Integer.MIN_VALUE;
long aboveMaxInt = 1L + Integer.MAX_VALUE;
String input = "[ "+Long.MAX_VALUE+","+Long.MIN_VALUE+","+aboveMaxInt+", "+belowMinInt+" ]";
long belowMaxLong = -1L + Long.MAX_VALUE;
long aboveMinLong = 1L + Long.MIN_VALUE;
String input = "[ "+Long.MAX_VALUE+","+Long.MIN_VALUE+","+aboveMaxInt+", "+belowMinInt+","+belowMaxLong+", "+aboveMinLong+" ]";
JsonParser p = createParser(jsonFactory(), mode, input);
assertToken(JsonToken.START_ARRAY, p.nextToken());
assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
Expand All @@ -297,6 +299,14 @@ public void testLongRange() throws Exception
assertEquals(JsonParser.NumberType.LONG, p.getNumberType());
assertEquals(belowMinInt, p.getLongValue());

assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
assertEquals(JsonParser.NumberType.LONG, p.getNumberType());
assertEquals(belowMaxLong, p.getLongValue());

assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
assertEquals(JsonParser.NumberType.LONG, p.getNumberType());
assertEquals(aboveMinLong, p.getLongValue());


assertToken(JsonToken.END_ARRAY, p.nextToken());
p.close();
Expand Down