Closed schlosna closed 1 year ago
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!
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?
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.
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 & https://github.com/openjdk/jdk/commit/f5d19b9109c7a0a40a85ca022d3089230ed1456b
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
.
Ok sounds good; will merge as soon as we get CLA done. Good work everyone!
I'll submit CLA ASAP, waiting on review right now
CLA submitted via email
Thank you @schlosna -- merged to be included in 2.15.0!
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 ofint[]
from its use ofjava.util.regex.Pattern.matcher(CharSequence)
to determine whether an input String is a zero offset and if so replace it withZ
.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
OpenJDK 17.0.6 on aarch64 Apple M1 Pro