Improve JWT parse / decode performance #620
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Return a new JWTDecodeException from private utility method `wrongNumberOfParts`, instead of throwing, since we throw from `splitToken()`.
|
cc @poovamraj |
|
Any reviewers out there....? |
|
Hey, @noetro sorry for the delayed response. Me and @jimmyjames will check this out this week and get back to you. Thanks for understanding |
|
No worries, I understand everyone is busy. I was just checking in. |
jimmyjames
approved these changes
Jan 31, 2023
There was a problem hiding this comment.
🎉 Thanks @noetro for this change, a great improvement! Appreciate it, and apologies for the delay in reviewing 🙇
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This merge improves JWT parse/decode performance by re-using Jackson ObjectFactory and ObjectReader instances that are thread safe and expensive to create.
To do so it also cleans up the deserialiser classes to use the information already available in the Jackson deserializer method parameters - namely the parser codec - instead of passing that information in at constructor time for the deserializer.
This type of pattern is already implemented in the serialize side of the codebase - note the static final ObjectMapper in JWTCreator and that the constructors for PayloadSerializer and HeaderSerializer take no arguments. This pull request brings parity to the deserialize side.
Note that there was some thought to already being able to re-use a parser by instantiating and holding an instance of 'com.auth0.jwt.JWT', but that only works partially since JWT.require is static and it instantiates a new JWTVerifier that always instantiates a new parser.
It's probably just safer to follow the same pattern as JWTCreator and transparently solve this in the library internally for the user.
I added JMH support to the build file with comments and one benchmark for decoding. On my machine throughput:
Master: 221755,008 ±(99.9%) 33951,905 ops/s [Average]
This Pull: 715079,328 ±(99.9%) 47281,796 ops/s [Average]
References
There is a lot of material out there on the internet on re-using ObjectMapper instances, and the JavaDoc in Jackson give guidance.
Testing
I added JMH support to the project so that there is some support for getting a performance impact picture. I didn't add new test cases as there seemed to be good coverage on all the deserialisation variants.
Checklist