-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Remove unnecessary content type checks #37646
Conversation
private static final String PRODUCES_CHECKED_PROPERTY_KEY = AbstractResteasyReactiveContext.CUSTOM_RR_PROPERTIES_PREFIX | ||
+ "ProducesChecked"; | ||
|
||
public void setProducesChecked(boolean checked) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is introducing a ConcurrentHashMap::put
there, but TBH it's still better than performing the check twice I think...
2 qq:
- ClassRouting vs FixedProduces handler's are always running in that order?
- Do we have other mechanism to store this information as a plain field somewhere instead, in the context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClassRouting vs FixedProduces handler's are always running in that order?
Yes.
Do we have other mechanism to store this information as a plain field somewhere instead, in the context?
We certainly can, but I was removing fields from the context because it was getting to big and we wanted to make it fit better in cache lines :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check with JOL, and if it is within some padding I am +1 to move it to a field, it sadly can matter in bench-marketing :"/ but need to be sure that's free
This comment has been minimized.
This comment has been minimized.
Waiting for a perf-lab run to see how this change affects performance |
Waiting the perf job to complete |
This comment has been minimized.
This comment has been minimized.
ok @geoand it seems that the performance now is right, and the behaviour too, but the ConcurrentHashMap::put in the hot path is just too costy; at the point where the cost is moved a bit toward it, see Check the |
Ouch... |
These checks have potentially already been performed in ClassRoutingHandler Closes: quarkusio#37637
I converted it to a regular field |
Rerunning the perf ci on this |
Thanks |
The perf CI is quite happy and we moved from 4 M req/sec to 4.3 M req/sec: now checking with JOL |
Super! |
JDK >= 15, COOPS, CCPS, 16 bytes align
Right now we have room for 15 bytes to fill, so we should be fine.
and the new one you're adding. Let me know and I can send a futrher pr later for it. |
Sure yeah, we can do that (I don't remember how many booleans we have, but it should be a few) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
These checks have potentially already
been performed in
ClassRoutingHandler
Closes: #37637