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

Avoid copy when parsing BigDecimal #798

Merged
merged 1 commit into from Jul 25, 2022

Conversation

marschall
Copy link
Contributor

Avoid a char[] copy when parsing a BigDecimal reducing intermediate
allocation.

Avoid a char[] copy when parsing a BigDecimal reducing intermediate
allocation.
@marschall
Copy link
Contributor Author

While profiling some Jackson code I noted that in order to create a BigDecimal we create a copy of the char[]. There is no need for this, BigDecimal has a char[],int,int constructor since Java 5. In the following micro benchmark I get about 20% throughput improvement.

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

}

@cowtowncoder cowtowncoder added the 2.14 Issue planned (at earliest) for 2.14 label Jul 24, 2022
@cowtowncoder
Copy link
Member

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).
The usual way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.
Once I get that, I'll merge this and it gets into upcoming 2.14.0.

Thank you again very much for providing this -- performance improvements are always welcome!

@marschall
Copy link
Contributor Author

@cowtowncoder awesome, give me one or two days

@marschall
Copy link
Contributor Author

@cowtowncoder I sent the CLA, let me know if you need anything more. Should I also submit a PR against FasterXML/jackson-benchmarks?

@cowtowncoder
Copy link
Member

CLA recieved, will merge.

@cowtowncoder cowtowncoder merged commit b5eb956 into FasterXML:2.14 Jul 25, 2022
@cowtowncoder cowtowncoder changed the title Avoid copy when parsing BigDecimal Avoid copy when parsing BigDecimal Jul 25, 2022
cowtowncoder added a commit that referenced this pull request Jul 25, 2022
}
chars = Arrays.copyOfRange(chars, off, off+len);
Copy link
Member

@pjfanning pjfanning Jul 25, 2022

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.

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

@cowtowncoder
Copy link
Member

@marschall Merged. As to jackson-benchmarks not sure; this might be little bit too specific to be tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.14 Issue planned (at earliest) for 2.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants