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

🐛fix: update getOffer to consider quality and specificity #2486

Conversation

sixcolors
Copy link
Member

@sixcolors sixcolors commented May 30, 2023

Description

Updates the getOffer helper function so that content negotiation follows https://www.rfc-editor.org/rfc/rfc9110#name-content-negotiation-fields, specifically that quality values are considered (also where q=0 type is not acceptable) and that specificity is also considered in a tie, as a final tie breaker client order is used.

Fixes #2387

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I tried to make my code as fast as possible with as few allocations as possible
  • For new code I have written benchmarks so that they can be analyzed and improved

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

@sixcolors
Copy link
Member Author

Considering client header priorities will have a performance impact. See discussion in #2387

@ReneWerner87
Copy link
Member

will check this tomorrow

Copy link
Member

@renanbastos93 renanbastos93 left a comment

Choose a reason for hiding this comment

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

Awesome, you did a good job, but I've considered two ideas to optimize your loop. Does that makes sense to you?

helpers.go Show resolved Hide resolved
helpers.go Show resolved Hide resolved
@ReneWerner87
Copy link
Member

ReneWerner87 commented Jun 6, 2023

@sixcolors looks very good 🫶

now also understand why it is slower

go test -v ./... -run=^$ -bench=Benchmark_Ctx_Accep -benchmem -count=3
Benchmark Compare:
Benchmark Iterations Old Time/op Old Iterations New Time/op New Time Difference
Benchmark_Ctx_Accepts/run-[]string{".xml"} 10142302 118.2 ns 5461294 269.5 ns +151.3 ns
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"} 8570805 139.2 ns 4463113 336.8 ns +197.6 ns
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"} 11842911 102.6 ns 5620503 230.4 ns +127.8 ns
Benchmark_Ctx_AcceptsCharsets 27449364 43.69 ns 9985008 125.8 ns +82.11 ns
Benchmark_Ctx_AcceptsEncodings 20565212 58.69 ns 6822858 172.5 ns +113.81 ns
Benchmark_Ctx_AcceptsLanguages 26086476 46.76 ns 4485744 270.4 ns +223.64 ns
Benchmark Old(master):
Benchmark Iterations Time/op Memory/op Allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"} 9641742 119.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 9895292 118.7 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 10142302 118.2 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 8538528 138.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 8570805 139.2 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 8351029 138.9 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 11645302 100.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 11745900 100.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 11842911 102.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 26744871 44.81 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 26279208 44.58 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 27449364 43.69 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 20362580 58.60 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 20565212 58.69 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 20394204 60.13 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 26086476 46.76 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12
Benchmark New(PR):
Benchmark Iterations Time/op Memory/op Allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 4082938 255.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 5157444 267.1 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 5461294 269.5 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 4373982 340.9 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 4463113 336.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 4062819 310.2 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 4183508 258.7 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 5620503 230.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 5073018 211.0 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 9523818 136.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 9985008 125.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 9398264 159.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 5713509 195.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 6536851 179.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 6822858 172.5 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 4556613 272.1 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 4455133 278.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 4485744 270.4 ns 0 B/op 0 allocs/op

you have to parse all headers first and can't run through them piece by piece because you need the order
before only the first ones were parsed and then matched with the customer's input

-> so the slight performance loss is okay for me
(you could also get rid of it by checking if the whole string contains "q=" at the beginning and then using the pass-through logic from before -> where we only parse parts)

can you still check the review hints in the PR

otherwise it would be good if we add more test scenarios with multiple quality factors and maybe a benchmark for the getOffer method, which is the core for many others

summary from above:

  • check the PR hints
  • consider the possibility to run a string without quality identifier with the old loop concept
  • add some test scenarios with strings with several qualities, for the main and getOffer method
  • add a benchmark for the core method(getOffer) so that we can test optimizations in the future

helpers.go Outdated Show resolved Hide resolved
helpers.go Show resolved Hide resolved
helpers.go Outdated Show resolved Hide resolved
helpers.go Show resolved Hide resolved
@sixcolors
Copy link
Member Author

sixcolors commented Jun 6, 2023

  • consider the possibility to run a string without quality identifier with the old loop concept

Specificity still matters even without the q value. Might not be worth the extra loops through the header to check for q and specificity...

@ReneWerner87
Copy link
Member

Can you share some benchmark numbers after these changes?
Master vs pull request

@sixcolors
Copy link
Member Author

sixcolors commented Jun 6, 2023

Can you share some benchmark numbers after these changes?
Master vs pull request

go test -v ./... -run=^$ -bench=Benchmark_Ctx_Accep -benchmem -count=3
Benchmark Compare:
Benchmark Iterations Old Time/op Old Iterations New Time/op New Time Difference
Benchmark_Ctx_Accepts/run-[]string{".xml"} 14128427 84.07 ns 8947115 130.6 ns +46.53 ns
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"} 11784786 101.1 ns 6741577 177.8 ns +76.7 ns
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"} 15576163 76.06 ns 9009831 132.1 ns +56.04 ns
Benchmark_Ctx_AcceptsCharsets 40155767 29.15 ns 17825068 67.42 ns +38.27 ns
Benchmark_Ctx_AcceptsEncodings 24537265 48.22 ns 11950063 100.4 ns +52.18 ns
Benchmark_Ctx_AcceptsLanguages 39774213 29.28 ns 7444000 160.8 ns +131.52 ns
Benchmark Old(master):
Benchmark Iterations Time/op Memory/op Allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"} 14128427 84.07 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 14058120 83.88 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 14375080 84.11 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 14045703 84.24 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 14034806 84.06 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"} 11784786 101.1 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 11851578 101.0 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 11822319 101.2 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 11654176 101.0 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 11811072 101.0 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"} 15576163 76.06 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 15525631 75.94 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 15594296 76.25 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 15636002 76.06 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 15548724 75.98 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets 40155767 29.15 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 40543222 29.77 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 39439466 28.50 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 40780608 29.80 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 39859770 28.51 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings 24537265 48.22 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 24918106 48.44 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 24050204 48.42 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 25031266 47.89 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 24149485 48.14 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages 39774213 29.28 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 39904504 29.45 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 39634644 29.18 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 39830498 29.23 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 39727206 29.27 ns 0 B/op 0 allocs/op
Benchmark New(PR):
Benchmark Iterations Time/op Memory/op Allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"} 8947115 130.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 8148916 130.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 9242019 130.9 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 9136976 130.0 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{".xml"}-12 9260547 130.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"} 6741577 177.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 6753759 177.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 6774225 177.5 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 6663565 178.3 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-12 6774759 177.9 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"} 9009831 132.1 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 9149313 130.6 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 8960199 132.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 8954554 132.5 ns 0 B/op 0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-12 8975256 132.3 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets 17825068 67.42 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 17584864 66.64 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 17816126 69.00 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 17753830 67.45 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsCharsets-12 11787234 101.3 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings 11950063 100.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 11753605 100.1 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 11852026 102.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 11593240 102.2 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsEncodings-12 7473263 161.0 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages 7444000 160.8 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 7385431 161.1 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 7547332 161.5 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 7516288 160.4 ns 0 B/op 0 allocs/op
Benchmark_Ctx_AcceptsLanguages-12 7516288 160.4 ns 0 B/op 0 allocs/op

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

Small typo in the test, but overall LGTM

ctx_test.go Outdated Show resolved Hide resolved
@ReneWerner87
Copy link
Member

@sixcolors can you do the last change with the typo

@ReneWerner87 ReneWerner87 merged commit 0f5ffed into gofiber:master Jun 7, 2023
@sixcolors sixcolors deleted the 2387-ctx-accepts-get-offer-q-and-specificity branch June 8, 2023 02:15
@sixcolors sixcolors restored the 2387-ctx-accepts-get-offer-q-and-specificity branch August 10, 2023 17:02
@sixcolors sixcolors deleted the 2387-ctx-accepts-get-offer-q-and-specificity branch March 27, 2024 01:18
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.

ctx.Accepts() quality values and specificity
4 participants