Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid use of json.NewDecoder #313

Merged
merged 4 commits into from
Aug 15, 2023
Merged

Avoid use of json.NewDecoder #313

merged 4 commits into from
Aug 15, 2023

Conversation

craigpastro
Copy link
Contributor

Avoid use of json.NewDecoder if not needed.

I was hanging around here and I noticed #303. This is an attempt to address it. Please let me know what you think. Thanks!

Resolves #303.

Avoid use of json.NewDecoder if not needed.

Resolves #303.
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in theory. The extra almost duplicated code is a bit annoying, but the performance gain should be visible. I had some numbers in the original issue, but I will try to do some performance comparisons with this next patch week.

parser.go Outdated Show resolved Hide resolved
parser.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numbers look good. As expected, we have a good performance gain (and slightly less allocations).

Before:

BenchmarkParseUnverified/map_claims-8         	 1497346	       792.5 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#01-8      	 1343728	       886.2 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/map_claims#02-8      	 1270246	       939.6 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/map_claims#03-8      	 1211380	      1045 ns/op	    2447 B/op	      46 allocs/op
BenchmarkParseUnverified/map_claims#04-8      	 1528545	       787.7 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#05-8      	 1519468	       796.2 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#06-8      	 1500554	       822.2 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#07-8      	 1485224	       814.9 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#08-8      	 1446320	       830.3 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#09-8      	 1475530	       815.5 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#10-8      	 1493253	       848.2 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#11-8      	 1475796	       818.6 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#12-8      	 1433794	       817.1 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#13-8      	 1292407	       945.9 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/map_claims#14-8      	 1301379	       941.3 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/map_claims#15-8      	 1000000	      1033 ns/op	    2448 B/op	      46 allocs/op
BenchmarkParseUnverified/map_claims#16-8      	 1293612	       925.4 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/registered_claims-8  	 1417246	       844.8 ns/op	    2231 B/op	      35 allocs/op
BenchmarkParseUnverified/registered_claims#01-8         	 1394887	       868.4 ns/op	    2280 B/op	      39 allocs/op
BenchmarkParseUnverified/registered_claims#02-8         	 1286151	       950.9 ns/op	    2344 B/op	      42 allocs/op
BenchmarkParseUnverified/registered_claims#03-8         	 1447845	       822.4 ns/op	    2208 B/op	      34 allocs/op
BenchmarkParseUnverified/registered_claims#04-8         	 1294418	       934.2 ns/op	    2336 B/op	      41 allocs/op
BenchmarkParseUnverified/registered_claims#05-8         	 1378839	       853.6 ns/op	    2224 B/op	      35 allocs/op
BenchmarkParseUnverified/registered_claims#06-8         	 1390922	       860.2 ns/op	    2224 B/op	      35 allocs/op

After:

BenchmarkParseUnverified/map_claims-8         	 1917262	       668.2 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#01-8      	 1561243	       714.7 ns/op	    1600 B/op	      38 allocs/op
BenchmarkParseUnverified/map_claims#02-8      	 1691029	       785.0 ns/op	    1592 B/op	      38 allocs/op
BenchmarkParseUnverified/map_claims#03-8      	 1548421	       836.3 ns/op	    1696 B/op	      43 allocs/op
BenchmarkParseUnverified/map_claims#04-8      	 1874676	       614.6 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#05-8      	 1958278	       630.7 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#06-8      	 1834294	       611.8 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#07-8      	 1884421	       638.2 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#08-8      	 1887714	       653.2 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#09-8      	 1747687	       680.3 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#10-8      	 1887813	       658.6 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#11-8      	 1830204	       647.6 ns/op	    1464 B/op	      32 allocs/op
BenchmarkParseUnverified/map_claims#12-8      	 1420887	       898.6 ns/op	    2208 B/op	      35 allocs/op
BenchmarkParseUnverified/map_claims#13-8      	 1247702	       953.2 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/map_claims#14-8      	 1000000	      1060 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/map_claims#15-8      	 1000000	      1077 ns/op	    2448 B/op	      46 allocs/op
BenchmarkParseUnverified/map_claims#16-8      	 1227679	      1009 ns/op	    2344 B/op	      41 allocs/op
BenchmarkParseUnverified/registered_claims-8  	 1281345	       897.1 ns/op	    2231 B/op	      35 allocs/op
BenchmarkParseUnverified/registered_claims#01-8         	 1312928	       902.9 ns/op	    2280 B/op	      39 allocs/op
BenchmarkParseUnverified/registered_claims#02-8         	 1236802	      1110 ns/op	    2344 B/op	      42 allocs/op
BenchmarkParseUnverified/registered_claims#03-8         	 1448676	       838.2 ns/op	    2208 B/op	      34 allocs/op
BenchmarkParseUnverified/registered_claims#04-8         	 1000000	      1041 ns/op	    2336 B/op	      41 allocs/op
BenchmarkParseUnverified/registered_claims#05-8         	 1692842	       685.4 ns/op	    1480 B/op	      32 allocs/op
BenchmarkParseUnverified/registered_claims#06-8         	 1792794	       669.0 ns/op	    1480 B/op	      32 allocs/op

For example registered_claims#05-8 and registered_claims#06-8 are the ones not using JSON number, and those seem to be almost 20% faster and less allocations (32 vs 35)

parser.go Show resolved Hide resolved
@oxisto oxisto mentioned this pull request Aug 14, 2023
10 tasks
@oxisto oxisto merged commit 78e25d6 into golang-jwt:main Aug 15, 2023
7 checks passed
@craigpastro craigpastro deleted the avoid-json-newdecoder branch August 15, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid use of json.NewDecoder if not needed
4 participants