✨ feat: add support for parameters in content negotiation #2678
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 |
|
Ah, I see the CI includes go 1.17, but fuzzing was introduced in 1.18. |
|
Another thing to note is that this does NOT support RFC 2231, which the stdlib mime package uses for ParseMediaType. mime.ParseMediaType is comparativesly slow and results in more allocations, but might simplify the code significantly (and give fiber RFC 2231 support). To give perspective, it's about 4-5 times slower than getHeaderValParams on my PC. On the other hand, it's on the order of about a microsecond or 2 difference, so it might be worth it. Dunno. Express's res.format doesn't seem to support it (I did only a very quick test). |
That seems reasonable. @ReneWerner87 opinion? |
|
Comparison of the existing Benchmark_Ctx_Accepts (with no parameters): Bench Master Bench PR No allocs, but clearly slower. I can look into additional micro-optimizations if a 50% increase in ns/op is unacceptable. |
move to a different file and only run with 1.18+
pls try to optimize it |
|
I have made some optimizations to the normal case and params case. I will do another pass later to look into simplifying the code. I'll also try to find more optimizations. I have found two "sweet spots" in terms of performance so far. For my information, which would be preferred? One is a bit faster for the normal case, and one has a stronger zero-alloc guarantee. Latest commit (8c7e42d)Slightly Faster for normal case, but with allocs when params are used (e0f7591) |
nickajacks1
force-pushed
the
slices-solution
branch
from
8c7e42d
to
85c131e
Compare
No need to answer, I got better results, still keeping zero allocs. BenchStill a bit worse than master, but much better than before. I'd imagine I'm beginning to approach the limit of how optimized the new code can get. I will do another look-over later for further polish. If this PR seems like it might be mergeable, I'll start looking at updating the docs. |
|
I won’t have time to review until early next week. |
|
@nickajacks1 Why copy |
There's no valid reason to be copying a function from go native libraries. It that function gets changed/fixed we would miss those changes. Original is here https://github.com/golang/go/blob/master/src/net/textproto/reader.go#L663 please import that one |
|
@gaby Is there an exported version of that function? I copied it because it's unexported. |
Doesnt seem like it. Have we check how much performance difference there is between utils.GetMime and using native Go (this func https://github.com/golang/go/blob/master/src/net/textproto/reader.go#L632 ) ? |
You're probably thinking about mime.TypeByExtension. There's already a benchmark comparing the two in utils/http_test.go. The TypeByExtension is twice as slow. In general, the mime package takes care of a lot of what we'd need for MIME and parameter parsing, but it's slow and allocates memory. |
|
The benchmarks seem to be improving 😁 @nickajacks1 please check failing CI tests. |
Attempts to approach the level of support offered by express, but behavior may differ in unusual corner cases. Some key behaviors from Express that are implemented: - If an offer does not have every parameter listed in the given Accept, it is rejected. - Parameters do not affect specificity. - In a given specificity, more parameters gives greater precedence - Parameters are unordered - Matching is case-insensitive - Surrounding quotes for parameter values are stripped - If an Accept type specifies a parameter more than once, the last value provided is taken. - Parameters after q are not processed. https://www.rfc-editor.org/rfc/rfc9110#name-parameters
nickajacks1
force-pushed
the
slices-solution
branch
from
85c131e
to
21e7b98
Compare
|
I wasn't sure what to do about this:
This doesn't seem to be triggered for the other Benchmark_* and Test_* methods...? @gaby did you want me to do something about |
nickajacks1
force-pushed
the
slices-solution
branch
from
21e7b98
to
26a0b14
Compare
In the comment add URL that points to the original source code. It's the one i posted above |
I tend to agree with the linter, Tests should be TestWhatever, BenchmarkWhatever and FuzzWhatever. |
sixcolors
approved these changes
Oct 31, 2023
There was a problem hiding this comment.
@nickajacks1 I'm seeing about 10% slowdown in Benchmark_Utils_GetOffer, awesome work!
Made a few testing related suggestions, otherwise LGTM!
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
sixcolors
approved these changes
Oct 31, 2023
|
Thanks, I'll implement the suggested changes and improve coverage this weekend. |
|
@nickajacks1 can you complete (or remove non applicable) checklist items from the PR description? |
Done. Also added details of some quirks that were not implemented in this PR. |
|
@ReneWerner87 @gaby @efectn PR LGTM. I approved before. |
|
Ok will look tomorrow |
ReneWerner87
reviewed
Nov 5, 2023
|
@nickajacks1 can you please share benchmarks again showing the master compared to the current state of the existing functionalities |
Bench AcceptsmasterPRBench getOffermasterPR |
ReneWerner87
approved these changes
Nov 6, 2023
I'm okay with the speed, but is there a way to keep this loss even smaller so that there is no loss of performance for the old cases? |
|
Sure, I'll take another look and let you know what I find. |
gaby
approved these changes
Nov 7, 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 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempts to approach the level of support offered by express, but behavior may differ in unusual corner cases.
Some key behaviors from Express that are implemented:
Behaviors that were intentionally not implemented:
Justification: this only affects functionality when a client escapes a character other than " or \ and the server doesn't account for it in the call to
ctx.Accepts. This case is unusual, and RFC 9110 states that clients ought not to do this. See doc comment for paramsMatch for more details.Accept: */*; param=foo, text/plain; q=0.9, text/plain; param=foo; q=0.8. If the server acceptstext/plainandtext/plain;param=foo, Express will match "text/plain" because the client explicitly stated they prefertext/plainovertext/plain;param=fooeven though*/*;param=foohas quality 1.0.Justification: this behavior was already present in the existing code.
Behaviors were mostly reverse engineered from res.format in Express.
https://www.rfc-editor.org/rfc/rfc9110#name-parameters
More Detail
We redundantly parse parameters for each offer N times instead of just once. Parsing them once in getOffer and passing the results to isAccepted indeed speeds things up, but it slows down the "normal" case (no parameters) more than I would like.
Type of change
Checklist: