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

fixes #2016 - make IP() and IPs() more reliable #2020

Merged
merged 3 commits into from Aug 23, 2022

Conversation

gbolo
Copy link
Contributor

@gbolo gbolo commented Aug 17, 2022

see #2016

@welcome
Copy link

welcome bot commented Aug 17, 2022

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

ctx.go Outdated
// extractValidIPs will return a slice of strings that represent valid IP addresses
// in the input string. The order is maintained. The separator is a comma
func extractValidIPs(input string) (validIPs []string) {
unvalidatedIps := strings.Split(input, ",")
Copy link
Member

Choose a reason for hiding this comment

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

this leads to further allocations
what does the benchmark say about the adjustment ?

please try to minimize the allocations so that we keep our perfomance promise for the consumers

@ReneWerner87
Copy link
Member

@gbolo can you try to make it more allocation less ? to improve the performance

@gbolo
Copy link
Contributor Author

gbolo commented Aug 19, 2022

@gbolo can you try to make it more allocation less ? to improve the performance

@ReneWerner87 I can try. Do you have any suggestions on an approach? Previously for the length of the ips slice, it was easy to figure out because you simply just counted the numbers of commas + 1:

ips = make([]string, bytes.Count(header, []byte(","))+1)

However, it's a little trickier now because we need to validate the input first. I will try another approach and run benchmarks to measure the impact.

@ReneWerner87
Copy link
Member

image
the old approach is not possible ?
or something like this

fiber/ctx.go

Lines 173 to 180 in 1fec875

commaPos = strings.IndexByte(header, ',')
if commaPos != -1 {
spec = utils.Trim(header[:commaPos], ' ')
} else {
spec = utils.TrimLeft(header, ' ')
}
if factorSign := strings.IndexByte(spec, ';'); factorSign != -1 {
spec = spec[:factorSign]

without storing the ip in a separate slice

@gbolo
Copy link
Contributor Author

gbolo commented Aug 22, 2022

I have improved the performance and reduced allocations of this functionality. I will continue to improve it more this week. I was also thinking that it may be a good idea to allow IP validation to be a configuration option. What do you think @ReneWerner87 ?

Current performance:

❯ go test -v -run=ctx_test.go -bench=Benchmark_Ctx_IP -benchmem -count=4
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2
cpu: 11th Gen Intel(R) Core(TM) i7-11700 @ 2.50GHz
Benchmark_Ctx_IPs
Benchmark_Ctx_IPs-16                    	 4877410	       278.6 ns/op	      96 B/op	       4 allocs/op
Benchmark_Ctx_IPs-16                    	 4073689	       271.5 ns/op	      96 B/op	       4 allocs/op
Benchmark_Ctx_IPs-16                    	 3561628	       316.0 ns/op	      96 B/op	       4 allocs/op
Benchmark_Ctx_IPs-16                    	 3180038	       323.1 ns/op	      96 B/op	       4 allocs/op

Previous performance with high allocations:

❯ go test -v -run=ctx_test.go -bench=Benchmark_Ctx_IPs -benchmem -count=4
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2
cpu: 11th Gen Intel(R) Core(TM) i7-11700 @ 2.50GHz
Benchmark_Ctx_IPs
Benchmark_Ctx_IPs-16    	 2446430	       769.3 ns/op	     256 B/op	      10 allocs/op
Benchmark_Ctx_IPs-16    	 1783593	       675.9 ns/op	     256 B/op	      10 allocs/op
Benchmark_Ctx_IPs-16    	 1708234	       587.8 ns/op	     256 B/op	      10 allocs/op
Benchmark_Ctx_IPs-16    	 1299441	       797.3 ns/op	     256 B/op	      10 allocs/op

Existing performance with NO ip validation

❯ go test -v -run=ctx_test.go -bench=Benchmark_Ctx_IP -benchmem -count=4
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2
cpu: 11th Gen Intel(R) Core(TM) i7-11700 @ 2.50GHz
Benchmark_Ctx_IPs
Benchmark_Ctx_IPs-16    	15274425	       119.9 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs-16    	11612115	       118.6 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs-16    	 9998650	       132.9 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs-16    	11643238	       137.8 ns/op	      48 B/op	       1 allocs/op

@ReneWerner87
Copy link
Member

thank you, would say ip validation is not needed for now

@ReneWerner87
Copy link
Member

are you finished or do you want to make adjustments ? otherwise i would merge

@gbolo
Copy link
Contributor Author

gbolo commented Aug 22, 2022

are you finished or do you want to make adjustments ? otherwise i would merge

please don't merge yet, I want to make some more adjustments

@gbolo
Copy link
Contributor Author

gbolo commented Aug 22, 2022

@ReneWerner87 I have completed the feature now. The performance of validation is improved, and also it is now an optional configuration flag. It is disabled by default to maintain identical performance and functionality as the previous releases. I also added many new unit tests and benchmark tests to track the feature.

❯ go test -v -run=ctx_test.go -bench=Benchmark_Ctx_IP -benchmem -count=4
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2
cpu: 11th Gen Intel(R) Core(TM) i7-11700 @ 2.50GHz
Benchmark_Ctx_IPs
Benchmark_Ctx_IPs-16                                      	12388728	       126.2 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs-16                                      	12268546	       112.5 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs-16                                      	10826390	       116.6 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs-16                                      	 8129878	       125.1 ns/op	      48 B/op	       1 allocs/op
Benchmark_Ctx_IPs_With_IP_Validation
Benchmark_Ctx_IPs_With_IP_Validation-16                   	 5226472	       280.9 ns/op	     112 B/op	       4 allocs/op
Benchmark_Ctx_IPs_With_IP_Validation-16                   	 4221264	       304.6 ns/op	     112 B/op	       4 allocs/op
Benchmark_Ctx_IPs_With_IP_Validation-16                   	 3878158	       318.5 ns/op	     112 B/op	       4 allocs/op
Benchmark_Ctx_IPs_With_IP_Validation-16                   	 3726033	       337.9 ns/op	     112 B/op	       4 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader
Benchmark_Ctx_IP_With_ProxyHeader-16                      	53945664	        21.56 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader-16                      	54460886	        21.68 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader-16                      	54627837	        21.61 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader-16                      	54638284	        21.67 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader_and_IP_Validation
Benchmark_Ctx_IP_With_ProxyHeader_and_IP_Validation-16    	11863418	       111.6 ns/op	      32 B/op	       2 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader_and_IP_Validation-16    	12253748	       126.4 ns/op	      32 B/op	       2 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader_and_IP_Validation-16    	 9589237	       108.4 ns/op	      32 B/op	       2 allocs/op
Benchmark_Ctx_IP_With_ProxyHeader_and_IP_Validation-16    	11862289	       105.1 ns/op	      32 B/op	       2 allocs/op
Benchmark_Ctx_IP
Benchmark_Ctx_IP-16                                       	55541400	        25.80 ns/op	       8 B/op	       1 allocs/op
Benchmark_Ctx_IP-16                                       	42441290	        27.46 ns/op	       8 B/op	       1 allocs/op
Benchmark_Ctx_IP-16                                       	52422422	        27.95 ns/op	       8 B/op	       1 allocs/op
Benchmark_Ctx_IP-16                                       	52316480	        23.30 ns/op	       8 B/op	       1 allocs/op
PASS
ok  	github.com/gofiber/fiber/v2	29.597s

to enable this feature simply enable it with config:

app := New(Config{EnableIPValidation: true})

Thanks for your help in reviewing this feature @ReneWerner87

@ReneWerner87
Copy link
Member

For new features don't forget to create a doc entry in our documentation repository.

@gbolo
Copy link
Contributor Author

gbolo commented Aug 23, 2022

For new features don't forget to create a doc entry in our documentation repository.

done: gofiber/docs#272

@ReneWerner87 ReneWerner87 merged commit 8540d0a into gofiber:master Aug 23, 2022
@welcome
Copy link

welcome bot commented Aug 23, 2022

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

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.

None yet

2 participants