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

Skip performing content negotiation twice #37637

Closed
franz1981 opened this issue Dec 9, 2023 · 7 comments · Fixed by #37646
Closed

Skip performing content negotiation twice #37637

franz1981 opened this issue Dec 9, 2023 · 7 comments · Fixed by #37646
Labels
area/rest kind/enhancement New feature or request
Milestone

Comments

@franz1981
Copy link
Contributor

franz1981 commented Dec 9, 2023

Description

And

if (accept.contains(mediaTypeString) || accept.contains("*/*") || accept.contains(mediaTypeSubstring)) {

Both seems to verify that Accept and (some) mediaType matches, but the former just to send back a proper response (if unmatched), while the other to find the appropriate writer.

Implementation ideas

Probably would be better to merge them in one pass (on class routing) and just save somewhere the matched one, to allow the subsequent to pick it as a starting point to make faster verifying if the writer is fine.

Moreover, a first pass in the matching logic can be to find for wildcard types on accept, to skip any subsequent op.

@franz1981 franz1981 added the kind/enhancement New feature or request label Dec 9, 2023
@franz1981
Copy link
Contributor Author

@geoand

@geoand
Copy link
Contributor

geoand commented Dec 9, 2023

👌

@geoand
Copy link
Contributor

geoand commented Dec 11, 2023

@franz1981 do you have some sample code that I can use to reproduce this and see its effect in a flamegraph?

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 11, 2023

/cc @FroMage (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

@geoand
Copy link
Contributor

geoand commented Dec 11, 2023

I am trying a change now

geoand added a commit to geoand/quarkus that referenced this issue Dec 11, 2023
These checks have potentially already
been performed in ClassRoutingHandler

Closes: quarkusio#37637
geoand added a commit to geoand/quarkus that referenced this issue Dec 11, 2023
These checks have potentially already
been performed in ClassRoutingHandler

Closes: quarkusio#37637
@geoand
Copy link
Contributor

geoand commented Dec 11, 2023

#37646 should do the trick

@franz1981
Copy link
Contributor Author

@geoand I think that's fine, buddy, thanks for looking into it: I'll answer in the PR than

geoand added a commit to geoand/quarkus that referenced this issue Dec 11, 2023
These checks have potentially already
been performed in ClassRoutingHandler

Closes: quarkusio#37637
geoand added a commit to geoand/quarkus that referenced this issue Dec 11, 2023
These checks have potentially already
been performed in ClassRoutingHandler

Closes: quarkusio#37637
geoand added a commit that referenced this issue Dec 11, 2023
Remove unnecessary content type checks
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 11, 2023
franz1981 pushed a commit to franz1981/quarkus that referenced this issue Dec 12, 2023
These checks have potentially already
been performed in ClassRoutingHandler

Closes: quarkusio#37637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants