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 String constructor directly for short ascii values #344

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

brharrington
Copy link
Contributor

On jdk9+ the String internals changed and it is more efficient to just use the String contructor that takes a byte array when dealing with ASCII values. Since jdk11 is the most recent LTS version with these changes and there are some methods added to String that can be used to detect it, this change uses the String constructor directly when on a newer JDK. The old behavior is preserved for jdk8.

Fixes #342

On jdk9+ the String internals changed and it is more efficient
to just use the String contructor that takes a byte array when
dealing with ASCII values. Since jdk11 is the most recent LTS
version with these changes and there are some methods added to
String that can be used to detect it, this change uses the
String constructor directly when on a newer JDK. The old
behavior is preserved for jdk8.
@brharrington
Copy link
Contributor Author

From my tests it seems to maintain roughly the same performance on jdk8 and shows improvements for jdk11+.

Benchmark                       Mode  Cnt       Score       Error   Units

java 8 baseline                thrpt    5  348058.136 ± 29095.348   ops/s
java 8 with change             thrpt    5  342844.462 ± 32386.599   ops/s

java 11 baseline               thrpt    5  313255.865 ± 43280.554   ops/s
java 11 with change            thrpt    5  347304.547 ± 44594.978   ops/s

java 17 baseline               thrpt    5  322069.060 ± 28697.702   ops/s
java 17 with change            thrpt    5  427788.567 ± 25312.986   ops/s

@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Nov 20, 2022
@cowtowncoder
Copy link
Member

Looks good, and I think can be merged in 2.14 for 2.14.1 patch.

One thing I may need: if we don't yet have a CLA (I feel we might have but didn't see one?), it'd be from here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the usual way is to print it out, fill & sign, scan/photo, email to info at fasterxml dot com.
Once we have it (please LMK if you did send it earlier and I'll try to locate it) I can go ahead merge this nice improvement in.

@cowtowncoder cowtowncoder added 2.14 and removed cla-needed PR looks good (although may also require code review), but CLA needed from submitter labels Nov 21, 2022
@cowtowncoder cowtowncoder modified the milestones: 2.10.0, 2.14.1 Nov 21, 2022
@cowtowncoder cowtowncoder merged commit 3a6fec9 into FasterXML:2.14 Nov 21, 2022
@brharrington brharrington deleted the short-ascii-opt branch November 21, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible performance improvement on jdk9+ for Smile decoding
2 participants