Skip to content
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

Use fast parser (FDP) for large BigDecimals (500+ chars) #1157

Merged
merged 1 commit into from Dec 8, 2023

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Dec 6, 2023

Gets rid of pre-existing custom code.
Uses the fastdoubleparser code for the 500+ chars case. This lib is already supported by jackson-core. It has better testing than our custom code. It is faster than our custom code (I have benchmarks in https://github.com/pjfanning/jackson-number-parse-bench).

We have had issues like this reported about the custom code:

@cowtowncoder
Copy link
Member

Sounds good to me. But I wonder if we have issue for the original introduction of this alternate parser.
PR #851 enabled "Big Number" parsing in general so maybe that's how this got in?

@pjfanning
Copy link
Member Author

@cowtowncoder BigDecimalParser was added by #677 and #678 and there were no real tests added in these PRs.

fastdoubleparser was added later and I think it is a better way to go.

@cowtowncoder cowtowncoder changed the title use fast parser for large bigdecimals (500+ chars) Use fast parser (FDP) for large BigDecimals (500+ chars), Dec 7, 2023
@@ -36,7 +36,7 @@ public static BigDecimal parse(final char[] chars, final int off, final int len)
if (len < 500) {
return new BigDecimal(chars, off, len);
}
return parseBigDecimal(chars, off, len, len / 10);
return JavaBigDecimalParser.parseBigDecimal(chars, off, len);
Copy link
Member

Choose a reason for hiding this comment

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

But doesn't this force use of "fast" BigDecimal parser for all cases? Looking at NumberInput.parseBigDecimal() we call parse() when StreamReadFeature.USE_FAST_BIG_NUMBER_PARSER is disabled (default), and parseWithFastParser() if enabled.

So I think this method should just become:

  return new BigDecimal(chars, off, len);

(and possibly existing try-catch block, although that is sort of questionable... except I think some calls are made from jackson-databind, from TokenBuffer f.ex so maybe that is legit).

Copy link
Member Author

Choose a reason for hiding this comment

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

this line is preceded by an if block that says when there are less than 500 chars, return new BigDecimal(chars, off, len)

The reason why we don't want to use return new BigDecimal(chars, off, len) for 500+ chars is that this call gets very slow when you have lots of chars. JavaBigDecimalParser is faster. The custom code with the problems is not quite as fast but is still faster than return new BigDecimal(chars, off, len).

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to try to keep the perf improvement that #677 brought in but just to do it with better and safer code.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. So, I think what you are saying is that:

  1. Yes, now >500 character default (USE_FAST_BIG_NUMBER_PARSER = false) would go to same code as (USE_FAST_BIG_NUMBER_PARSER = true) BUT
  2. That's ok because we want to protect users against non-linear slow-down for very long legal numbers?

and I guess that makes sense even if pedantically speaking we shouldn't do that (wrt setting USE_FAST_BIG_NUMBER_PARSER). Especially since case of 500 or more characters really is very niche usage, most likely only used by malicious users.

With that, I can merge this for 2.17.

@cowtowncoder cowtowncoder changed the title Use fast parser (FDP) for large BigDecimals (500+ chars), Use fast parser (FDP) for large BigDecimals (500+ chars) Dec 8, 2023
@cowtowncoder cowtowncoder merged commit 4518d17 into FasterXML:2.17 Dec 8, 2023
5 checks passed
cowtowncoder added a commit that referenced this pull request Dec 8, 2023
cowtowncoder added a commit that referenced this pull request Dec 9, 2023
@pjfanning pjfanning deleted the use-fast-parser-big-decs branch December 10, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants