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

Possible performance improvement on jdk9+ for Smile decoding #342

Closed
brharrington opened this issue Nov 12, 2022 · 2 comments · Fixed by #344
Closed

Possible performance improvement on jdk9+ for Smile decoding #342

brharrington opened this issue Nov 12, 2022 · 2 comments · Fixed by #344
Milestone

Comments

@brharrington
Copy link
Contributor

We have a use-case for smile that involves parsing a lot of short ASCII string values and _decodeShortAsciiValue shows up prominently in profiles. This implementation copies the input bytes into a character array which is then used to create the String. On newer Java versions there is a significant improvement by just using the String constructor that takes a byte array (sample change). This is because In jdk9 and later the internal representation of String changed (JEP 254) and creating from an ASCII byte array can basically just do Arrays.copyOfRange. Unfortunately, this approach seems to be significantly slower on jdk8.

Results from some quick benchmarks using JMH:

Benchmark                       Mode  Cnt       Score       Error   Units

java 8 baseline                thrpt    5  344448.375 ± 46864.690   ops/s
java 8 with change             thrpt    5  194250.482 ±  7374.829   ops/s

java 17 baseline               thrpt    5  301991.539 ± 52160.837   ops/s
java 17 with change            thrpt    5  413394.519 ± 16857.477   ops/s

I'm assuming it would be unacceptable to take that hit on java 8 at this time. Are there any existing examples in jackson where behavior is conditional on the java version used?

@cowtowncoder
Copy link
Member

Interesting. That would be a nice optimization to use.

There are places where JDK level is being used, detected indirectly by existence (or lack) of classes:

  • JDK 7 type support; still dynamic for Android
  • Record support (2.12+ I think)

In this case it'd probably make sense to try to dynamically look for a method of String that was added in JDK 9, set up a flag in smile decoder and select path to take. Or maybe implementation of an abstract class for decode operation.

I may not have time to work on this myself, although I think it sounds like worthy thing to add. But if you (or anyone else) had time and interest, I could help get a PR merged for 2.15 (or maybe even 2.14 if simple enough).

brharrington added a commit to brharrington/spectator that referenced this issue Nov 18, 2022
@cowtowncoder cowtowncoder added this to the 2.14.1 milestone Nov 21, 2022
@cowtowncoder cowtowncoder changed the title Possible performance improvement on jdk9+ Possible performance improvement on jdk9+ for Smile decoding Nov 21, 2022
cowtowncoder added a commit that referenced this issue Nov 21, 2022
@cowtowncoder
Copy link
Member

FWTW, Jackson 3 will require some post-JDK8 baseline (17 or 21); that's where we could probably make change without any checking on baseline (since all versions it runs on benefit from change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants