search

dgrijalva / jwt-go

ARCHIVE - Golang implementation of JSON Web Tokens (JWT). This project is now maintained at:
https://github.com/golang-jwt/jwt
MIT License
10.78k stars 997 forks source link

Allocation optimization #455

Open jkanywhere opened 3 years ago

jkanywhere commented 3 years ago

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.

amnonbc commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 https://github.com/golang-jwt/jwt/pull/33 instead.