Skip to content
This repository has been archived by the owner on May 21, 2022. It is now read-only.

Allocation optimization #455

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allocation optimization #455

wants to merge 4 commits into from

Conversation

jkanywhere
Copy link

@jkanywhere jkanywhere commented Feb 27, 2021 •
edited
Loading

Add benchmark coverage of ECDSA signing methods.

Reduce memory allocations for ECDSA signing by using big.Int.FillBytes instead of allocating and copying bytes directly.
FillBytes was added in go 1.15.

FillBytes sets buf to the absolute value of x, storing it as a zero-extended big-endian byte slice, and returns buf.

Use base64.RawURLEncoding to avoid having to add and remove = padding.

RawURLEncoding ... is the same as URLEncoding but omits padding characters.

@jkanywhere
Test to ensure ECDSA signature is valid
d9e1c41 
Add assertions to ensure ECDSA signing methods return valid signatures.

This is probably covered elsewhere as well, but putting it in
ecdsa_test.go makes it more obvious and easier to find.
@jkanywhere
Benchmark ECDSA signing methods
fa71ffb 
Add benchmark coverage of ECDSA signing methods.

Benchmarks are run using the existing helper for comparability with
existing benchmarks.

Sign method is also tested directly, to avoid the overhead of *Token.

Report allocations for all benchmarks.

Allocation count for ES384 and ES512 fluctuate across test runs, the
other singing consistently report the same number of allocations.

Sample output:
```
$ go test -bench=Bench -run=NONE .
2021/02/26 18:18:30 Listening...
goos: darwin
goarch: amd64
pkg: github.com/dgrijalva/jwt-go
BenchmarkECDSASigning/Basic_ES256-8                   	  190572	      6702 ns/op	    4249 B/op	      65 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8         	   47383	     24650 ns/op	    3329 B/op	      43 allocs/op
BenchmarkECDSASigning/Basic_ES384-8                   	    1113	   1252975 ns/op	 1750744 B/op	   14474 allocs/op
BenchmarkECDSASigning/Basic_ES384/sign-only-8         	     286	   3937773 ns/op	 1746175 B/op	   14423 allocs/op
BenchmarkECDSASigning/Basic_ES512-8                   	     662	   1949937 ns/op	 3028386 B/op	   19608 allocs/op
BenchmarkECDSASigning/Basic_ES512/sign-only-8         	     170	   6856189 ns/op	 3025471 B/op	   19571 allocs/op
BenchmarkECDSASigning/basic_ES256_invalid:_foo_=>_bar-8         	  190638	      6665 ns/op	    4249 B/op	      65 allocs/op
BenchmarkHS256Signing-8                                	 1000000	      1024 ns/op	    1584 B/op	      32 allocs/op
BenchmarkHS384Signing-8                                	  917286	      1447 ns/op	    1969 B/op	      32 allocs/op
BenchmarkHS512Signing-8                                	  827744	      1470 ns/op	    2065 B/op	      32 allocs/op
BenchmarkRS256Signing-8                                	    3037	    390077 ns/op	   32576 B/op	     136 allocs/op
BenchmarkRS384Signing-8                                	    2976	    379155 ns/op	   32684 B/op	     136 allocs/op
BenchmarkRS512Signing-8                                	    3205	    388628 ns/op	   32704 B/op	     136 allocs/op
```
@jkanywhere
Reduce allocations during ECDSA signing
0866ae7 
Reduce the number of byte arrays allocated by using big.Int.FillBytes
when calculating ECDSA signature.

After this change, Benchmarks of ES256 signing method consistently
report 4 fewer allocations.

Before:
```
BenchmarkECDSASigning/Basic_ES256-8              190572         6702 ns/op       4249 B/op         65 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8     47383        24650 ns/op       3329 B/op         43 allocs/op
```

After:
```
BenchmarkECDSASigning/Basic_ES256-8              187682         6725 ns/op       4121 B/op         61 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8     48656        24446 ns/op       3201 B/op         39 allocs/op
```
@jkanywhere
Use base64.RawURLEncoding to avoid padding
efac53c 
JWT uses a non-padded base64 encoding.

Current code uses base64.URLEncoding to generate a padded string and
then removes the padding.
Likewise, current code adds padding before decoding.

Instead, use base64.RawURLEncoding which does not add or require the
padding in the first place.

In addition to making the code cleaner, this reduces memory allocations
as reported by benchmarks.

Before:
```
BenchmarkECDSASigning/Basic_ES256-8                     191396         6917 ns/op       4121 B/op         61 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8            49347        25039 ns/op       3201 B/op         39 allocs/op
BenchmarkECDSASigning/basic_ES256_invalid:_foo_=>_bar-8 190668         6586 ns/op       4121 B/op         61 allocs/op
BenchmarkHS256Signing-8                                1260060         1131 ns/op       1585 B/op         32 allocs/op
BenchmarkHS384Signing-8                                 861378         1387 ns/op       1969 B/op         32 allocs/op
BenchmarkHS512Signing-8                                 896745         1463 ns/op       2065 B/op         32 allocs/op
BenchmarkRS256Signing-8                                   3086       355769 ns/op      32576 B/op        136 allocs/op
BenchmarkRS384Signing-8                                   3414       353570 ns/op      32694 B/op        136 allocs/op
BenchmarkRS512Signing-8                                   3235       349394 ns/op      32706 B/op        136 allocs/op
```

After:
```
BenchmarkECDSASigning/Basic_ES256-8                     176617         6827 ns/op       4021 B/op         58 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8            48038        24213 ns/op       3169 B/op         38 allocs/op
BenchmarkECDSASigning/basic_ES256_invalid:_foo_=>_bar-8 194352         6928 ns/op       4021 B/op         58 allocs/op
BenchmarkHS256Signing-8                                1000000         1127 ns/op       1488 B/op         29 allocs/op
BenchmarkHS384Signing-8                                 972552         1369 ns/op       1873 B/op         29 allocs/op
BenchmarkHS512Signing-8                                 780751         1368 ns/op       1969 B/op         29 allocs/op
BenchmarkRS256Signing-8                                   3014       387326 ns/op      32475 B/op        133 allocs/op
BenchmarkRS384Signing-8                                   3044       361411 ns/op      32591 B/op        133 allocs/op
BenchmarkRS512Signing-8                                   3273       355504 ns/op      32607 B/op        133 allocs/op
```

Benchmarks of signing methods ES384 and ES512 are omitted because their
allocations are not consistent.
@amnonbc
Copy link

amnonbc commented Jun 15, 2021

The ECDSA slowness has caused me pain too.
This is a nice fix.

Is the maintainer still around?
Or should I fork my own copy of the repo to get this.

@oxisto
Copy link

oxisto commented Jun 20, 2021

@jkanywhere This sounds interesting. Can we interest you in bringing this over to http://github.com/golang-jwt/jwt with a new PR?

This project is not maintained anymore and we decided to do a community fork/clone of it. See #462

@amnonbc
Copy link

amnonbc commented Jun 20, 2021

This fix requires Go 1.15

So merging this necessitates removing support for Go 1.14 and below
(which is something we can do anyway - as versions <= 1.14 of the Go toolchain and libs
are unsupported).

@amnonbc amnonbc mentioned this pull request Jun 20, 2021
Closed
@amnonbc
Copy link

amnonbc commented Jun 20, 2021 •
edited
Loading

Before:

goos: darwin
goarch: amd64
pkg: github.com/golang-jwt/jwt
cpu: Intel(R) Core(TM) i5-8257U CPU @ 1.40GHz
BenchmarkECDSASigning/Basic_ES256-8         	  175926	      6826 ns/op	    3985 B/op	      64 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8         	   45678	     26038 ns/op	    3097 B/op	      42 allocs/op
BenchmarkECDSASigning/Basic_ES384-8                   	    1084	   1133196 ns/op	 1748536 B/op	   14458 allocs/op
BenchmarkECDSASigning/Basic_ES384/sign-only-8         	     339	   3518124 ns/op	 1743932 B/op	   14406 allocs/op
BenchmarkECDSASigning/Basic_ES512-8                   	     652	   1926562 ns/op	 3028133 B/op	   19608 allocs/op
BenchmarkECDSASigning/Basic_ES512/sign-only-8         	     195	   6106483 ns/op	 3021383 B/op	   19545 allocs/op
BenchmarkECDSASigning/basic_ES256_invalid:_foo_=>_bar-8         	  170736	      6828 ns/op	    3985 B/op	      64 allocs/op

cpu: Intel(R) Core(TM) i5-8257U CPU @ 1.40GHz
BenchmarkECDSASigning
BenchmarkECDSASigning/Basic_ES256-8         	  146896	      7532 ns/op	    3861 B/op	      58 allocs/op
BenchmarkECDSASigning/Basic_ES256/sign-only-8         	   43806	     27549 ns/op	    2945 B/op	      37 allocs/op
BenchmarkECDSASigning/Basic_ES384-8                   	    1100	   1077246 ns/op	 1748908 B/op	   14456 allocs/op
BenchmarkECDSASigning/Basic_ES384/sign-only-8         	     343	   3514156 ns/op	 1749466 B/op	   14447 allocs/op
BenchmarkECDSASigning/Basic_ES512-8                   	    2709	    435354 ns/op	    9122 B/op	      87 allocs/op
BenchmarkECDSASigning/Basic_ES512/sign-only-8         	     668	   1771388 ns/op	    8111 B/op	      66 allocs/op
BenchmarkECDSASigning/basic_ES256_invalid:_foo_=>_bar-8         	  159170	      6977 ns/op	    3860 B/op	      58 allocs/op

@lggomez
Copy link

lggomez commented Jul 4, 2021

@amnonbc @oxisto If the owners of this requests do not answer in an adequate period (maybe 2 to 4 weeks) we can port those we're interested in continuing to work to the new repo with a notice CC'ing to them in case they want to re create them with their accounts (credit for the original work should of course go to them in the commit titles and comments)

I can port the PRs, from the one's I see here that do no involve documentation the ones that look the less disruptive and mergeable/reviewable (at least IMO) are the following:

@jkanywhere
Copy link
Author

@jkanywhere This sounds interesting. Can we interest you in bringing this over to http://github.com/golang-jwt/jwt with a new PR?

Yes. Please use golang-jwt/jwt#33 instead.

@jkanywhere jkanywhere mentioned this pull request Jul 6, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jkanywhere @amnonbc @oxisto @lggomez