-
-
Notifications
You must be signed in to change notification settings - Fork 795
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 BigDecimal
s (500+ chars)
#1157
Use fast parser (FDP) for large BigDecimal
s (500+ chars)
#1157
Conversation
Sounds good to me. But I wonder if we have issue for the original introduction of this alternate parser. |
@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. |
BigDecimal
s (500+ chars),
@@ -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); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Yes, now >500 character default (USE_FAST_BIG_NUMBER_PARSER = false) would go to same code as (USE_FAST_BIG_NUMBER_PARSER = true) BUT
- 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.
BigDecimal
s (500+ chars), BigDecimal
s (500+ chars)
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:
BigDecimalParser
#967