Optimize InstantDeserializer method replaceZeroOffsetAsZIfNecessary()

[schlosna]

While reviewing some JFRs from some services that heavily use Jackson for deserializing timestamps, I noticed that com.fasterxml.jackson.datatype.jsr310.deser.InstantDeserializer.replaceZeroOffsetAsZIfNecessary(String) was allocating a lot of int[] from its use of java.util.regex.Pattern.matcher(CharSequence) to determine whether an input String is a zero offset and if so replace it with Z.

I would like to propose an alternative implementation to avoid expensive regex matcher allocations when replacing zero offset with 'Z'.

Using a JMH benchmark with a variety of generated timestamps I see the following results on x64 (~7x speedup) and aarch64 (~2.5x speedup):

OpenJDK 17.0.6 on x64 Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz

Benchmark                           Mode  Cnt    Score   Error  Units
InstantDeserializerBenchmark.index  avgt    5   19.543 ± 1.384  ns/op
InstantDeserializerBenchmark.regex  avgt    5  155.081 ± 3.234  ns/op

OpenJDK 17.0.6 on aarch64 Apple M1 Pro

Benchmark                           Mode  Cnt   Score   Error  Units
InstantDeserializerBenchmark.index  avgt    5  16.855 ± 0.537  ns/op
InstantDeserializerBenchmark.regex  avgt    5  58.155 ± 1.444  ns/op

[cowtowncoder]

Excellent, thank you @schlosna! I especially appreciate jmh benchmark here.

Just one small thing before I can merge this for inclusion in 2.15.0: unless I have asked for a CLA, I'd need it from:

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

and the usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
This only needs to be done once before the first contribution so if you already sent it once let me know.

Thank you again & looking forward to merging this PR!

[cowtowncoder]

Ok, one oddity: new test cases fail on JDK 8 and 11 (see CI run; I had to enable it since it's your first contribution). Possibly related to handling (or not) of colon in time offset parts?

[schlosna]

Thanks for the quick review @cowtowncoder ! I'll get the CLA reviewed & signed hopefully tomorrow.

I removed the commit with additional test cases as I forgot those will fail on JDK 8 & 11 (the test cases also fail on JDK 8 & 11 applied to origin/2.15). I had run that locally against origin/2.15 and this branch locally with JDK 17 and both passed all tests. Let me know if you'd prefer I rework the tests to cover additional zone offsets, I just need to refresh my memory of how various JDKs handle the offset parsing.

[schlosna]

After some needed sleep, I realized why those tests pass on JDK 17+ but did not pass on JDK 8 & 11 and it is the reason we're replacing the zero offset with Z in the first place -- before JDK 12 DateTimeFormatter.ISO_INSTANT didn't handle offsets. This was added by JDK-8166138 & openjdk/jdk@f5d19b9

I've updated the test cases in InstantDeserTest to assume JDK 12+ for those cases, and validated that the OffsetDateTimeDeserTest and ZonedDateTimeDeserTest also cover replaceZeroOffsetAsZIfNecessary.

[carterkozak]

Can we optimize this using String.regionMatches?
perhaps using something along the lines of

switch (remaining) {
    case 2:
       return text.regionmatches..."00" ? text.substring(0, plusIndex) + 'Z' : text;
     case 4:
        ..etc
     case 5:
       ..etc
 }
 return text;

[schlosna]

Seems reasonable to avoid the substring allocation, updated to use regionMatches & switch and reran JMH and tested locally with JDKs 8, 11, 17.

Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
JDK 17.0.5, OpenJDK 64-Bit Server VM, 17.0.5+8-LTS
Benchmark                           Mode  Cnt    Score   Error  Units
InstantDeserializerBenchmark.index  avgt    5   20.003 ± 0.486  ns/op
InstantDeserializerBenchmark.regex  avgt    5  156.174 ± 7.654  ns/op

Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
JDK 11.0.16.1, OpenJDK 64-Bit Server VM, 11.0.16.1+9-LTS
Benchmark                           Mode  Cnt    Score   Error  Units
InstantDeserializerBenchmark.index  avgt    5   20.818 ± 1.267  ns/op
InstantDeserializerBenchmark.regex  avgt    5  192.248 ± 7.341  ns/op

Apple M1 Pro aarch64
JDK 17.0.6, OpenJDK 64-Bit Server VM, 17.0.6+10-LTS
Benchmark                           Mode  Cnt   Score   Error  Units
InstantDeserializerBenchmark.index  avgt    5  17.498 ± 0.677  ns/op
InstantDeserializerBenchmark.regex  avgt    5  58.096 ± 1.330  ns/op

Apple M1 Pro aarch64
Benchmark                           Mode  Cnt   Score   Error  Units
InstantDeserializerBenchmark.index  avgt    5  16.176 ± 7.842  ns/op
InstantDeserializerBenchmark.regex  avgt    5  63.201 ± 5.668  ns/op

[cowtowncoder]

Ok sounds good; will merge as soon as we get CLA done. Good work everyone!

[schlosna]

I'll submit CLA ASAP, waiting on review right now

[schlosna]

CLA submitted via email

[cowtowncoder]

Thank you @schlosna -- merged to be included in 2.15.0!