ARTEMIS-6010 - RBAC authorization speedup#6370
Conversation
| } else { | ||
| logger.trace("user is NOT authorized"); | ||
|
|
||
| if (logger.isTraceEnabled()) { |
There was a problem hiding this comment.
For this simple logger call there is generally no benefit to checking if trace is enabled as the first thing the implementation does is check if trace is enabled before stringifying the argument. We have done work in the past to remove all these unneeded isEnabled calls and I don't think we want to start adding them back
There was a problem hiding this comment.
I thought that the arguments are stringified even if the actual output to console/log does not happen. And like mentioned in jira, it seemed that using isTraceEnabled (consistently) produces faster execution times. Maybe a helper function would be a better idea, or maybe all the gains would be lost that way. I agree that adding isEnabled everywhere seems like the wrong solution.
I don't mind removing this, and it shouldn't be me who decides about this anyway. But since it was already "there", I included it in this PR.
There was a problem hiding this comment.
When you use the logger.trace("message {}", arg) etc APIs SLF4J first checks internally if the level is enabled. The toString() method of the argument and the final string concatenation only occur if the trace level is active. In past experiments people have tested this extensively and the saving between just calling the logger vs gating every call with isXEnabled is optimistically fractions of nanoseconds.
Data for such experiments can be found, some are here https://github.com/wsargent/slf4j-benchmark/blob/master/README.md
I would remove the is enabled check and just leave the change to make it a single call removing the current if / else
There was a problem hiding this comment.
Thank you for the explaination and the link!
I'll make the changes and force push.
| if (authorized) { | ||
| logger.trace("user is authorized"); | ||
| } else { | ||
| logger.trace("user is NOT authorized"); |
There was a problem hiding this comment.
I think it would probably be best to move this block into SecurityManagerUtil.authorize itself rather than repeating it in two places.
| logger.trace("user is NOT authorized"); | ||
| } | ||
|
|
||
| logger.trace("user is authorized: {}", authorized); |
There was a problem hiding this comment.
In my opinion this makes the logging a bit less clear, easier to misread in a hurry.
3 participants
No description provided.