-
-
Notifications
You must be signed in to change notification settings - Fork 812
Avoid copy when parsing BigDecimal
#798
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
Conversation
Avoid a char[] copy when parsing a BigDecimal reducing intermediate allocation.
While profiling some Jackson code I noted that in order to create a @OutputTimeUnit(TimeUnit.SECONDS)
public class JacksonStdBigDecimalReadVanilla
{
private static final JsonFactory JSON_FACTORY = new JsonFactory();
private static final String JSON;
static {
StringBuilder json = new StringBuilder();
json.append('[');
for (int i = 0; i < 1_000; i++) {
if (i > 0) {
json.append(',');
}
json.append("123.456");
}
json.append(']');
JSON = json.toString();
}
@Benchmark
public void parseBigDecimal(Blackhole blackhole) throws IOException {
try (JsonParser parser = JSON_FACTORY.createParser(JSON)) {
parser.nextToken(); // start array
JsonToken nextToken = parser.nextToken();
while (nextToken == JsonToken.VALUE_NUMBER_FLOAT) {
blackhole.consume(parser.getDecimalValue());
nextToken = parser.nextToken();
}
}
}
} |
Thank you for contributing this patch @marschall -- interesting that we had overlooked existing of that constructor, good catch! I'd be happy to merge this, and the only thing I need (if not already done earlier) is to get CLA from: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf it's only needed once before the first Jackson contribution, good for all future contributions (hence if you have sent one earlier LMK and I can verify). Thank you again very much for providing this -- performance improvements are always welcome! |
@cowtowncoder awesome, give me one or two days |
@cowtowncoder I sent the CLA, let me know if you need anything more. Should I also submit a PR against FasterXML/jackson-benchmarks? |
CLA recieved, will merge. |
BigDecimal
} | ||
chars = Arrays.copyOfRange(chars, off, 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.
Would it make more sense to avoid this Arrays.copyOfRange and change the BigDecimalParser constructor to take (chars, off, len)? It looks like the parseBigDecimal can be readily changed to support an offset and explicit 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.
I have a PR for this
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.
That could make sense if easy enough to do.
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.
@marschall Merged. As to |
Avoid a char[] copy when parsing a BigDecimal reducing intermediate
allocation.