Reduce cost of logging in REST Client #27074
Conversation
geoand
commented
Aug 2, 2022
•
geoand
commented
- Only execute code if debug logging is actually enabled
- Don't use lambdas
geoand
changed the title
| if (headers.isEmpty()) { | ||
| return ""; | ||
| } | ||
| StringBuilder sb = new StringBuilder((headers.size() * (6 + 1 + 6)) + (headers.size() - 1)); // this is a very rough estimate of a result like 'key1=value1 key2=value2' |
There was a problem hiding this comment.
I fail to see why we should make this more complicated if only used for debug logging? I can understand doing things like that in a hot path but here?
There was a problem hiding this comment.
Complicated how? It's just iterating over the headers and building up a String. How is that more complicated than was was being done with lambdas?
There was a problem hiding this comment.
14 lines vs 3 lines, pre-calculating the header size, iterating the entries, checking if the entry is the first, and so on... yes this has added complexity.
Sure, the streams, lambda, and String.format approach will be slower, but it's more readable, and if it's already guarded by isDebugEnabled() and is not in a hot path, then this might not add value in the final execution.
Having said that I appreciate that Quarkus tries to squeeze every ms of performance, but it might be better to focus on more critical paths.
There was a problem hiding this comment.
14 lines vs 3 lines, pre-calculating the header size, iterating the entries, checking if the entry is the first, and so on... yes this has added complexity.
I wouldn't say so, because it's code that anyone that knows even the slightest Java can write without any effort.
As for the header size pre-calculation, I honestly don't see why people are tripping over it - it's just a heuristic that easily could have been left out, but its application results in less allocations.
There was a problem hiding this comment.
Having said that I appreciate that Quarkus tries to squash every ms of performance, but it might be better to focus on more critical paths
I truly appreciate the input, but the squezing of performance out of Quarkus is an ongoing task. In this specific case I just happen to come across something that was not performant (this is important as I did not go hunting for performance issues and come up with this...) and I addressed it - it literally took 5 minutes, it's not like it took time from other tasks...
It's always good to keep in mind the context of how development works in a large project when attempting to apply generalized (but otherwise well intended) statements.
There was a problem hiding this comment.
I truly appreciate the input, but the squezing of performance out of Quarkus is an ongoing task. In this specific case I just happen to come across something that was not performant (this is important as I did not go hunting for performance issues and come up with this...) and I addressed it - it literally took 5 minutes, it's not like it took time from other tasks...
It's always good to keep in mind the context of how development works in a large project when attempting to apply generalized (but otherwise well intended) statements.
Thank you, I know this is a different kind of project and I didn't want to apply generalized statements, I was following the same observation of Guillaume, I can read the code perfectly (and probably everyone involved in the project can), but claiming that anyone that knows even the slightest Java can write without any effort, well, it's just not realistic.
Anyway, since the context of the project is different, it's perfectly fine. 👍🏽
