search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
316 stars 136 forks source link

Possible performance improvement on jdk9+ for Smile decoding #342

Closed brharrington closed 1 year ago

brharrington commented 2 years ago

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 commented 2 years ago

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:

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).

cowtowncoder commented 2 months ago

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).