MessageFormatter: small performance improvement for 1 and 2 object parameter cases?
Hi all, When I was looking at the issue with the Throwable parameter in the EventRecordingLogger (SLF4J-466[1]) I also stumbled upon an issue within the MessageFormatter. I opened SLF4J-468[2] for that. Ceki asked me to explain this issue to the list, since a change in this class is not something we should do lightly. There are two cases. Case 1: In case you pass one object to the format method[3] it will put that object in an Object[] and then the next method will call yet another method to see if that Object[]'s last element is a Throwable. And if so it will create a new Object[] containing 0 elements and it will try to do the formatting logic, even though there is nothing to format. My proposed changes: 2.0.0: - Directly call the actual message formatting function with an Object[1] argArray and null throwable.[4] The downside of this change is that if the one object format message is called with a Throwable you no longer get 0 arguments and a Stacktrace, you get 1 argument and no Stacktrace. But why would you call the format method if you already know there is nothing to format? 1.7.x: - Add an instanceof check to the object argument - Call the actual formatting function with either null as argArray or null as throwable and the actual argument on the other. - Extend the argArray == null check with || argArray.length == 0 and change the return to return new FormattingTuple(messagePattern, null, throwable); I think this last change should also be in the 2.0.0 branch, but it's not strictly required for this issue. But now the Throwable might get lost in case someone calls the actual formatting function with an empty argument array. Case 2: In case you pass two objects to the format method[5] it will put both objects in an Object[] and then the next method will call yet another method to see if the last Object in this Object[] is a Throwable. And if so it will create a new Object[] containing only the first element, which it copies from the original Object[]. And then it will do the normal formatting logic. My proposed changes, for both branches: - Add an instanceof check to the second object argument - Call the actual formatting function with either an Object[2] argArray and null throwable, or an Object[1] argArray and the throwable. Of course, it would even be nicer to not have to do the get last parameter and check if it's a Throwable and if so do some other magic then in the other case. But as a user of the logging framework the current way of working is very convenient and logical. So I guess we cannot change that in the current API but only in the new fluent API? I hope I explained the issue clearly. Kind regards, Wessel [1] https://jira.qos.ch/browse/SLF4J-466 [2] https://jira.qos.ch/browse/SLF4J-468 [3] https://www.slf4j.org/apidocs/org/slf4j/helpers/MessageFormatter.html#format...) [4] https://www.slf4j.org/apidocs/org/slf4j/helpers/MessageFormatter.html#arrayF...) [5] https://www.slf4j.org/apidocs/org/slf4j/helpers/MessageFormatter.html#format...)
The idea is to have technical discussions so that they are public. Thus, having it in Jira or on this mailing list is more or less equivalent. In particular because Jira comments are echoed on this mailing list as well. As such, please see my comment in https://jira.qos.ch/browse/SLF4J-468 On 13/08/2019 23:11, Wessel van Norel wrote:
Hi all,
When I was looking at the issue with the Throwable parameter in the EventRecordingLogger (SLF4J-466[1]) I also stumbled upon an issue within the MessageFormatter. I opened SLF4J-468[2] for that.
Ceki asked me to explain this issue to the list, since a change in this class is not something we should do lightly.
There are two cases.
Case 1: In case you pass one object to the format method[3] it will put that object in an Object[] and then the next method will call yet another method to see if that Object[]'s last element is a Throwable. And if so it will create a new Object[] containing 0 elements and it will try to do the formatting logic, even though there is nothing to format.
My proposed changes:
2.0.0: - Directly call the actual message formatting function with an Object[1] argArray and null throwable.[4]
The downside of this change is that if the one object format message is called with a Throwable you no longer get 0 arguments and a Stacktrace, you get 1 argument and no Stacktrace. But why would you call the format method if you already know there is nothing to format?
1.7.x: - Add an instanceof check to the object argument - Call the actual formatting function with either null as argArray or null as throwable and the actual argument on the other. - Extend the argArray == null check with || argArray.length == 0 and change the return to return new FormattingTuple(messagePattern, null, throwable);
I think this last change should also be in the 2.0.0 branch, but it's not strictly required for this issue. But now the Throwable might get lost in case someone calls the actual formatting function with an empty argument array.
Case 2: In case you pass two objects to the format method[5] it will put both objects in an Object[] and then the next method will call yet another method to see if the last Object in this Object[] is a Throwable. And if so it will create a new Object[] containing only the first element, which it copies from the original Object[]. And then it will do the normal formatting logic.
My proposed changes, for both branches: - Add an instanceof check to the second object argument - Call the actual formatting function with either an Object[2] argArray and null throwable, or an Object[1] argArray and the throwable.
Of course, it would even be nicer to not have to do the get last parameter and check if it's a Throwable and if so do some other magic then in the other case. But as a user of the logging framework the current way of working is very convenient and logical. So I guess we cannot change that in the current API but only in the new fluent API?
I hope I explained the issue clearly.
Kind regards, Wessel
[1] https://jira.qos.ch/browse/SLF4J-466 [2] https://jira.qos.ch/browse/SLF4J-468 [3] https://www.slf4j.org/apidocs/org/slf4j/helpers/MessageFormatter.html#format...) [4] https://www.slf4j.org/apidocs/org/slf4j/helpers/MessageFormatter.html#arrayF...) [5] https://www.slf4j.org/apidocs/org/slf4j/helpers/MessageFormatter.html#format...) _______________________________________________ slf4j-dev mailing list slf4j-dev@qos.ch http://mailman.qos.ch/mailman/listinfo/slf4j-dev
participants (2)
-
Ceki Gülcü -
Wessel van Norel