Performance improvements #255

skjolber · 2018-05-30T17:46:53Z

Changes from #194 plus various minor improvements.

crypto helper is more application-specific to JWT in taking in header and payload as different arguments.
passing ObjectReader around where it might be used
directly generate json in PayloadSerializer
default HashMap sizes

Stats:
Current:
Benchmark Mode Cnt Score Error Units
JavaJwtFromAuth0Bench.claim thrpt 5 14174 ± 297,734 ops/s
JavaJwtFromAuth0Bench.parse thrpt 5 63936 ± 1605,433 ops/s
JavaJwtFromAuth0Bench.validate thrpt 5 14085 ± 189,359 ops/s

Optimized:
Benchmark Mode Cnt Score Error Units
JavaJwtFromAuth0Bench.claim thrpt 5 17962 ± 1126,980 ops/s
JavaJwtFromAuth0Bench.parse thrpt 5 311501 ± 12945,965 ops/s
JavaJwtFromAuth0Bench.validate thrpt 5 18152 ± 755,550 ops/s

See also

https://github.com/skjolber/java-jwt-benchmark

skjolber · 2018-06-12T22:25:43Z

@lbalmaceda - I am looking at increasing code coverage so the checks pass, would it be acceptable to remove the Algorithm.sign(byte[]) method, or must it first be deprectated?

Edit: Went ahead and deleted unused code.

skjolber · 2018-06-26T16:38:30Z

@lbalmaceda any hope for merging this? Also the build fail on java 7 due to some connection reset and code coverage seems to be not working either(?).

…optimize

lbalmaceda

I'm slowly going through issues and PRs. Thanks for your participation so far. I've left a few comments! cheers

lbalmaceda · 2018-11-16T19:26:13Z

lib/src/main/java/com/auth0/jwt/algorithms/CryptoHelper.java


-    boolean verifySignatureFor(String algorithm, byte[] secretBytes, byte[] contentBytes, byte[] signatureBytes) throws NoSuchAlgorithmException, InvalidKeyException {
-        return MessageDigest.isEqual(createSignatureFor(algorithm, secretBytes, contentBytes), signatureBytes);
+    boolean verifySignatureFor(String algorithm, byte[] secretBytes, String header, String payload, byte[] signatureBytes) throws NoSuchAlgorithmException, InvalidKeyException {


Unless I'm missing something, this method could be safely deleted and you could use the byte[] header and payload variant instead.

See below comment

lbalmaceda · 2018-11-16T19:42:40Z

lib/src/main/java/com/auth0/jwt/algorithms/CryptoHelper.java

    }

-    boolean verifySignatureFor(String algorithm, PublicKey publicKey, byte[] contentBytes, byte[] signatureBytes) throws NoSuchAlgorithmException, InvalidKeyException, SignatureException {
+    boolean verifySignatureFor(String algorithm, PublicKey publicKey, String header, String payload, byte[] signatureBytes)  throws NoSuchAlgorithmException, InvalidKeyException, SignatureException {


Same as my comment above, unless this methods are called from many places passing String and as a convenience we want to wrap that here. What are your thoughts?

Well, the header/payload from DecodedJWT are strings, so it is good to pass them directly so to the helper. ~~Also there is quite a lot of unit test calls to this convenice method.~~ I guess keeping them around is fair.

lbalmaceda · 2018-11-16T19:56:57Z

lib/src/main/java/com/auth0/jwt/algorithms/Algorithm.java

     * @return the signature in a base64 encoded array of bytes
     * @throws SignatureGenerationException if the Key is invalid.
     */
-    public abstract byte[] sign(byte[] contentBytes) throws SignatureGenerationException;
+    public abstract byte[] sign(byte[] headerBytes, byte[] payloadBytes) throws SignatureGenerationException;


This is a breaking change (affecting implementations as well)

Yes. Basically this narrows the use for Algorithm to just signing tokens, which is in line with its own class description (in the javadoc).

I don't disagree that this is an improvement. But it will break customers who are implementing their own Algorithms.. I can't see many customers doing that but some feature requests have been opened in the past asking us to extend the algorithms we support. Let me discuss this internally. Maybe with a good readme note and proper changelog notes we can accept it. I can't promise anything.

Re above: No breaking change allowed (and no major plans for the time being). We need to find an alternative to this. Normally I'd say call the new method from the old one and deprecate it, but in this case there's no way for you to figure out at what point of the contentBytes array is the half of it in order to separate it into header and payload bytes. What if you add the new sign signature and keep both methods side-to-side? I guess the new one can call the old one by merging both header and payload bytes with a "." in-between, so not to repeat the sign method's body code. If not you will have to rollback the changes for this class.

I've added a default implementation of the new method (sign with header and payload) in Algorithm, but overridden that in the known subclasses. This should ensure backwards compatibility for those implementing their own algorithms, at the same time as speeding things up for the rest of us.

CryptoHelper has now both the new and the old methods, but they're all quite small.

@lbalmaceda If this solution is acceptable, I'll add a few of the old unit tests for code coverage (see failing build checks).

lbalmaceda · 2018-11-16T20:07:57Z

lib/src/main/java/com/auth0/jwt/impl/JWTParser.java

-        } catch (IOException e) {
-            throw exceptionForInvalidJson(json);
-        }
+    private static JWTDecodeException decodeException() {


there should be no reason to make this 2 methods static. Does this have any impact on the performance?

I prefer static to non-static if there is no access to fields; it is just simpler and in this case is there no kind of subclassing going on either.

lbalmaceda · 2018-11-16T20:17:05Z

lib/src/main/java/com/auth0/jwt/impl/PayloadImpl.java

    }

    @Override
    public Map<String, Claim> getClaims() {
-        Map<String, Claim> claims = new HashMap<>();
+        Map<String, Claim> claims = new HashMap<>(tree.size() * 2);


What's the rationale to pick this default value?

Crude estimate of necessary capacity to avoid rehashing and collision.

https://stackoverflow.com/questions/10901752/what-is-the-significance-of-load-factor-in-hashmap

lbalmaceda · 2018-11-16T20:26:03Z

lib/src/main/java/com/auth0/jwt/impl/PayloadSerializer.java

                    break;
                default:
+                    gen.writeFieldName(e.getKey());
                    if (e.getValue() instanceof Date) {


this default case could be merged with the 3 known claims above (which are dates as well), or rephrasing, this could be deleted:

case PublicClaims.EXPIRES_AT: case PublicClaims.ISSUED_AT: case PublicClaims.NOT_BEFORE: gen.writeFieldName(e.getKey()); gen.writeNumber(dateToSeconds((Date) e.getValue())); break;

Deleted according to your comment.

…optimize

skjolber · 2019-01-25T13:49:27Z

@lbalmaceda see updates / comments, sorry for slow reaction on QA.

lbalmaceda · 2019-01-28T14:32:05Z

lib/src/main/java/com/auth0/jwt/JWTVerifier.java

@@ -51,7 +54,7 @@ static Verification init(Algorithm algorithm) throws IllegalArgumentException {
            }

            this.algorithm = algorithm;
-            this.claims = new HashMap<>();
+            this.claims = new HashMap<>(48);


No magic numbers please. Can we move it into a constant and give it a proper name/description?

On second thought, 16 (the default) is probably okey here.

Agree. I've never seen a token with that many claims 😆

lbalmaceda · 2019-01-28T14:40:14Z

lib/src/main/java/com/auth0/jwt/algorithms/Algorithm.java

     * @return the signature in a base64 encoded array of bytes
     * @throws SignatureGenerationException if the Key is invalid.
     */
-    public abstract byte[] sign(byte[] contentBytes) throws SignatureGenerationException;
+    public abstract byte[] sign(byte[] headerBytes, byte[] payloadBytes) throws SignatureGenerationException;


Re above: No breaking change allowed (and no major plans for the time being). We need to find an alternative to this. Normally I'd say call the new method from the old one and deprecate it, but in this case there's no way for you to figure out at what point of the contentBytes array is the half of it in order to separate it into header and payload bytes. What if you add the new sign signature and keep both methods side-to-side? I guess the new one can call the old one by merging both header and payload bytes with a "." in-between, so not to repeat the sign method's body code. If not you will have to rollback the changes for this class.

lbalmaceda

Code is fine so yes, go ahead and add any test you consider appropriate to keep the coverage. The only comment is around the old vs new methods and the deprecated annotation. Some of them are missing the annotation and in particular for the CryptoHelper class, even if not public, please add the reason why it's deprecated (e.g. "please use this other alternative method ...") or at least add a comment like you did in the new sign implementation standing that the "duplicated code" in the new methods is until we decide to remove the other signatures. Just to keep track of this on the future. The rest is fine. Once checks pass I'll merge it. Thanks!

lib/src/main/java/com/auth0/jwt/algorithms/RSAAlgorithm.java

lib/src/main/java/com/auth0/jwt/algorithms/NoneAlgorithm.java

lib/src/main/java/com/auth0/jwt/algorithms/HMACAlgorithm.java

…thods

…rage

…e commons Charsets

skjolber · 2019-01-29T12:31:38Z

@lbalmaceda Added a few more unit tests, Javadoc in CryptoHelper, @deprecated to the old sign(..) methods, some cleanups.

lbalmaceda · 2019-01-29T16:51:16Z

Thanks a lot for sticking with this 👍 👍

skjolber · 2019-01-29T21:04:36Z

@lbalmaceda Thank you for your patience 👍 🥇

skjolber · 2019-02-01T09:26:06Z

@lbalmaceda FYI: https://github.com/skjolber/java-jwt-benchmark updated

lbalmaceda · 2019-02-01T15:49:48Z

@skjolber I forgot to ask you that the other day. Thanks! 💥

skjolber force-pushed the optimize branch from dabb2e4 to 14dc114 Compare May 30, 2018 17:49

skjolber force-pushed the optimize branch from 14dc114 to 29e9874 Compare June 7, 2018 10:50

Performance improvements

66e7263

skjolber force-pushed the optimize branch from 29e9874 to 66e7263 Compare June 7, 2018 10:53

skjolber added 2 commits June 14, 2018 01:03

Performance improvements

bd3ed2e

Remove unused code, improve tests.

d8b1cd7

skjolber force-pushed the optimize branch from 0193f5d to d8b1cd7 Compare June 13, 2018 23:23

skjolber force-pushed the optimize branch 2 times, most recently from f77fe56 to d8b1cd7 Compare June 26, 2018 16:32

Merge branch 'master' into optimize

e0fae69

skjolber force-pushed the optimize branch from 6ea0365 to e0fae69 Compare June 26, 2018 16:35

skjolber added 3 commits August 20, 2018 16:00

Merge branch 'master' into optimize

0fab631

Merge branch 'optimize' of https://github.com/skjolber/java-jwt into …

ed2c0de

…optimize

Merge branch 'master' into optimize

367f2c0

skjolber mentioned this pull request Sep 12, 2018

Improve Jackson ObjectMapper usage #142

Closed

lbalmaceda reviewed Nov 16, 2018

View reviewed changes

skjolber added 3 commits January 25, 2019 14:20

Handle PublicClaims of date types in switch default case

81d1d42

Merge branch 'optimize' of https://github.com/skjolber/java-jwt into …

5c63f94

…optimize

Merge branch 'master' into optimize

45f1cfb

lbalmaceda suggested changes Jan 28, 2019

View reviewed changes

skjolber added 3 commits January 28, 2019 16:07

Revert to using default HashMap capacity

b45b420

Restore support for Algorithm.sign(byte[]).

e42964b

Fix crypto test helper - allow minus (-) in token regexp

5e9c189

lbalmaceda reviewed Jan 28, 2019

View reviewed changes

lib/src/main/java/com/auth0/jwt/algorithms/RSAAlgorithm.java Show resolved Hide resolved

lib/src/main/java/com/auth0/jwt/algorithms/NoneAlgorithm.java Show resolved Hide resolved

lib/src/main/java/com/auth0/jwt/algorithms/HMACAlgorithm.java Show resolved Hide resolved

skjolber added 3 commits January 28, 2019 23:29

Add @deprecated annotation and unit tests for deprecated signature me…

b0190fe

…thods

Add some unit tests for the old signing method for improved code cove…

b9c2ac1

…rage

Improve Javadoc

6cda750

Formatting, remove unused imports, prefer StandardCharsets over apach…

4efdfba

…e commons Charsets

lbalmaceda approved these changes Jan 29, 2019

View reviewed changes

lbalmaceda merged commit 8ca949e into auth0:master Jan 29, 2019

lbalmaceda added the CH: Added label Jan 29, 2019

lbalmaceda modified the milestones: v3-Next, 3.7.0 Jan 29, 2019

skjolber deleted the optimize branch January 29, 2019 21:04

skjolber mentioned this pull request Feb 1, 2019

Using ObjectReader to cache Jackson bindings when parsing JWT #194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements #255

Performance improvements #255

skjolber commented May 30, 2018 •

edited

Loading

skjolber commented Jun 12, 2018 •

edited

Loading

skjolber commented Jun 26, 2018

lbalmaceda left a comment

lbalmaceda Nov 16, 2018

skjolber Jan 25, 2019

lbalmaceda Nov 16, 2018

skjolber Jan 25, 2019 •

edited

Loading

lbalmaceda Nov 16, 2018

skjolber Jan 25, 2019

lbalmaceda Jan 25, 2019

lbalmaceda Jan 28, 2019

skjolber Jan 28, 2019

skjolber Jan 28, 2019

lbalmaceda Nov 16, 2018

skjolber Jan 25, 2019 •

edited

Loading

lbalmaceda Nov 16, 2018

skjolber Jan 25, 2019

lbalmaceda Nov 16, 2018

skjolber Jan 25, 2019

skjolber commented Jan 25, 2019

lbalmaceda Jan 28, 2019

skjolber Jan 28, 2019

lbalmaceda Jan 28, 2019

lbalmaceda Jan 28, 2019

lbalmaceda left a comment •

edited

Loading

skjolber commented Jan 29, 2019

lbalmaceda commented Jan 29, 2019

skjolber commented Jan 29, 2019

skjolber commented Feb 1, 2019

lbalmaceda commented Feb 1, 2019

Performance improvements #255

Performance improvements #255

Conversation

skjolber commented May 30, 2018 • edited Loading

skjolber commented Jun 12, 2018 • edited Loading

skjolber commented Jun 26, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjolber Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjolber Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjolber commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment • edited Loading

Choose a reason for hiding this comment

skjolber commented Jan 29, 2019

lbalmaceda commented Jan 29, 2019

skjolber commented Jan 29, 2019

skjolber commented Feb 1, 2019

lbalmaceda commented Feb 1, 2019

skjolber commented May 30, 2018 •

edited

Loading

skjolber commented Jun 12, 2018 •

edited

Loading

skjolber Jan 25, 2019 •

edited

Loading

skjolber Jan 25, 2019 •

edited

Loading

lbalmaceda left a comment •

edited

Loading