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

Optimize basicauth allocations #2333

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Conversation

msaf1980
Copy link
Contributor

@msaf1980 msaf1980 commented Feb 13, 2023

Description

Fix for test basic header as described in RFC 7617, like

Authorization: Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==

Also avoid string buffer alloc in strings.ToLower if string not in lower case, like Basic

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

@welcome
Copy link

welcome bot commented Feb 13, 2023

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@efectn
Copy link
Member

efectn commented Feb 13, 2023

Can you post benchmark results here (old, new)

@msaf1980
Copy link
Contributor Author

Before (current master)

$ go test -benchmem -run=^$ -bench ^Benchmark_Middleware_BasicAuth github.com/gofiber/fiber/v2/middleware/basicauth
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/middleware/basicauth
cpu: Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
Benchmark_Middleware_BasicAuth-6         	 3374157	       356.2 ns/op	      80 B/op	       5 allocs/op
Benchmark_Middleware_BasicAuth_Upper-6   	 3117049	       387.5 ns/op	      80 B/op	       6 allocs/op
PASS
ok  	github.com/gofiber/fiber/v2/middleware/basicauth	3.168s

After (this PR)

$ go test -benchmem -run=^$ -bench ^Benchmark_Middleware_BasicAuth github.com/gofiber/fiber/v2/middleware/basicauth
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/middleware/basicauth
cpu: Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
Benchmark_Middleware_BasicAuth-6         	 3175054	       371.6 ns/op	      80 B/op	       5 allocs/op
Benchmark_Middleware_BasicAuth_Upper-6   	 3207657	       376.4 ns/op	      80 B/op	       5 allocs/op
PASS
ok  	github.com/gofiber/fiber/v2/middleware/basicauth	3.156s

As I see, if auth header like Authorization: basic QWxhZGRpbjpvcGVuIHNlc2FtZQ== we have some perf drawback.
But if request like Authorization: Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ== (as described in RFC, and most requests is) - we have minus one allocation.

@efectn
Copy link
Member

efectn commented Feb 13, 2023

Can you use https://github.com/gofiber/fiber/blob/master/utils/strings.go#L65 and send results again.

(Also utils.ToLower)

@msaf1980
Copy link
Contributor Author

msaf1980 commented Feb 13, 2023

With utils.EqualFold

$ go test -benchmem -run=^$ -bench ^Benchmark_Middleware_BasicAuth github.com/gofiber/fiber/v2/middleware/basicauth
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/middleware/basicauth
cpu: Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
Benchmark_Middleware_BasicAuth-6         	 3487176	       343.3 ns/op	      80 B/op	       5 allocs/op
Benchmark_Middleware_BasicAuth_Upper-6   	 3503778	       348.3 ns/op	      80 B/op	       5 allocs/op

With utils.ToLower

$ go test -benchmem -run=^$ -bench ^Benchmark_Middleware_BasicAuth github.com/gofiber/fiber/v2/middleware/basicauth
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/middleware/basicauth
cpu: Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
Benchmark_Middleware_BasicAuth-6         	 3373893	       358.1 ns/op	      80 B/op	       6 allocs/op
Benchmark_Middleware_BasicAuth_Upper-6   	 3374263	       356.8 ns/op	      80 B/op	       6 allocs/op

Update PR for use utils.EqualFold ?

@efectn
Copy link
Member

efectn commented Feb 13, 2023

With utils.EqualFold

$ go test -benchmem -run=^$ -bench ^Benchmark_Middleware_BasicAuth github.com/gofiber/fiber/v2/middleware/basicauth
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/middleware/basicauth
cpu: Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
Benchmark_Middleware_BasicAuth-6         	 3487176	       343.3 ns/op	      80 B/op	       5 allocs/op
Benchmark_Middleware_BasicAuth_Upper-6   	 3503778	       348.3 ns/op	      80 B/op	       5 allocs/op

With utils.ToLower

$ go test -benchmem -run=^$ -bench ^Benchmark_Middleware_BasicAuth github.com/gofiber/fiber/v2/middleware/basicauth
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/middleware/basicauth
cpu: Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz
Benchmark_Middleware_BasicAuth-6         	 3373893	       358.1 ns/op	      80 B/op	       6 allocs/op
Benchmark_Middleware_BasicAuth_Upper-6   	 3374263	       356.8 ns/op	      80 B/op	       6 allocs/op

Update PR for use utils.EqualFold ?

sure

@msaf1980
Copy link
Contributor Author

sure

Done. If you plan to merge, may be better squash commit on merge ?

@efectn
Copy link
Member

efectn commented Feb 13, 2023

sure

Done. If you plan to merge, may be better squash commit on merge ?

Don't need it. We're already merging squashed commits into the master

@ReneWerner87 ReneWerner87 merged commit 497eb02 into gofiber:master Feb 13, 2023
@welcome
Copy link

welcome bot commented Feb 13, 2023

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87 ReneWerner87 changed the title Basic auth alloc Optimize basicauth allocations Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants