RFR: 8153666: Optimize Formatter.formatMessage
Hi, Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless. More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-... (thanks Jason!) bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666 patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00 best regards, and thanks to all who provided suggestions and archeological background! -- daniel
Hi Daniel, Looks ok, but... Formatter.java: - line 104: the javadoc says it looks for '{0' but the implementation looks for '{'[0..9] It looks like the spec is out of sync with the implementation (the implementation is more lenient) and has been for a while. According to the spec/javadoc; no formatting should occur unless it contains "{0" Roger On 6/8/2016 9:46 AM, Daniel Fuchs wrote:
Hi,
Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless.
More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-...
(thanks Jason!)
bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666
patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00
best regards, and thanks to all who provided suggestions and archeological background!
-- daniel
Good point Roger! I was already wondering whether looking for {[0-9] instead of {[0-3] deserved a small mention in the release note. The fact that the spec needs updating confirms that this little behavior change probably needs to be mentioned. I will do the paperwork for the spec change. I don't think we need any switch to revert to the old behaviour, right? best regards, -- daniel On 08/06/16 15:14, Roger Riggs wrote:
Hi Daniel,
Looks ok, but...
Formatter.java:
- line 104: the javadoc says it looks for '{0' but the implementation looks for '{'[0..9] It looks like the spec is out of sync with the implementation (the implementation is more lenient) and has been for a while.
According to the spec/javadoc; no formatting should occur unless it contains "{0"
Roger
On 6/8/2016 9:46 AM, Daniel Fuchs wrote:
Hi,
Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless.
More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-...
(thanks Jason!)
bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666
patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00
best regards, and thanks to all who provided suggestions and archeological background!
-- daniel
Yes, I would update the spec to the new behavior. On 6/8/2016 10:31 AM, Daniel Fuchs wrote:
Good point Roger!
I was already wondering whether looking for {[0-9] instead of {[0-3] deserved a small mention in the release note.
The fact that the spec needs updating confirms that this little behavior change probably needs to be mentioned.
I will do the paperwork for the spec change. I don't think we need any switch to revert to the old behaviour, right?
best regards,
-- daniel
On 08/06/16 15:14, Roger Riggs wrote:
Hi Daniel,
Looks ok, but...
Formatter.java:
- line 104: the javadoc says it looks for '{0' but the implementation looks for '{'[0..9] It looks like the spec is out of sync with the implementation (the implementation is more lenient) and has been for a while.
According to the spec/javadoc; no formatting should occur unless it contains "{0"
Roger
On 6/8/2016 9:46 AM, Daniel Fuchs wrote:
Hi,
Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless.
More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-...
(thanks Jason!)
bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666
patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00
best regards, and thanks to all who provided suggestions and archeological background!
-- daniel
On 08/06/16 15:35, Roger Riggs wrote:
Yes, I would update the spec to the new behavior.
What about: * <li>Otherwise, if the string contains {@code "{<digit>"} * where {@code <digit>} is in {@code [0-9]} * then java.text.MessageFormat is used to format the string. -- daniel
On 6/8/2016 10:31 AM, Daniel Fuchs wrote:
Good point Roger!
I was already wondering whether looking for {[0-9] instead of {[0-3] deserved a small mention in the release note.
The fact that the spec needs updating confirms that this little behavior change probably needs to be mentioned.
I will do the paperwork for the spec change. I don't think we need any switch to revert to the old behaviour, right?
best regards,
-- daniel
On 08/06/16 15:14, Roger Riggs wrote:
Hi Daniel,
Looks ok, but...
Formatter.java:
- line 104: the javadoc says it looks for '{0' but the implementation looks for '{'[0..9] It looks like the spec is out of sync with the implementation (the implementation is more lenient) and has been for a while.
According to the spec/javadoc; no formatting should occur unless it contains "{0"
Roger
On 6/8/2016 9:46 AM, Daniel Fuchs wrote:
Hi,
Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless.
More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-...
(thanks Jason!)
bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666
patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00
best regards, and thanks to all who provided suggestions and archeological background!
-- daniel
+1 On 6/8/2016 10:37 AM, Daniel Fuchs wrote:
On 08/06/16 15:35, Roger Riggs wrote:
Yes, I would update the spec to the new behavior.
What about:
* <li>Otherwise, if the string contains {@code "{<digit>"} * where {@code <digit>} is in {@code [0-9]} * then java.text.MessageFormat is used to format the string. (The current phrasing does not use 'then').
-- daniel
On 6/8/2016 10:31 AM, Daniel Fuchs wrote:
Good point Roger!
I was already wondering whether looking for {[0-9] instead of {[0-3] deserved a small mention in the release note.
The fact that the spec needs updating confirms that this little behavior change probably needs to be mentioned.
I will do the paperwork for the spec change. I don't think we need any switch to revert to the old behaviour, right?
best regards,
-- daniel
On 08/06/16 15:14, Roger Riggs wrote:
Hi Daniel,
Looks ok, but...
Formatter.java:
- line 104: the javadoc says it looks for '{0' but the implementation looks for '{'[0..9] It looks like the spec is out of sync with the implementation (the implementation is more lenient) and has been for a while.
According to the spec/javadoc; no formatting should occur unless it contains "{0"
Roger
On 6/8/2016 9:46 AM, Daniel Fuchs wrote:
Hi,
Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless.
More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-...
(thanks Jason!)
bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666
patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00
best regards, and thanks to all who provided suggestions and archeological background!
-- daniel
Thanks Roger. Here is an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.01 best regards, -- daniel On 08/06/16 15:56, Roger Riggs wrote:
+1
On 6/8/2016 10:37 AM, Daniel Fuchs wrote:
On 08/06/16 15:35, Roger Riggs wrote:
Yes, I would update the spec to the new behavior.
What about:
* <li>Otherwise, if the string contains {@code "{<digit>"} * where {@code <digit>} is in {@code [0-9]} * then java.text.MessageFormat is used to format the string. (The current phrasing does not use 'then').
Looks good, Thanks, Roger On 6/8/2016 11:47 AM, Daniel Fuchs wrote:
Thanks Roger.
Here is an updated webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.01
best regards,
-- daniel
On 08/06/16 15:56, Roger Riggs wrote:
+1
On 6/8/2016 10:37 AM, Daniel Fuchs wrote:
On 08/06/16 15:35, Roger Riggs wrote:
Yes, I would update the spec to the new behavior.
What about:
* <li>Otherwise, if the string contains {@code "{<digit>"} * where {@code <digit>} is in {@code [0-9]} * then java.text.MessageFormat is used to format the string. (The current phrasing does not use 'then').
Thanks, Daniel. I only gave it a quick look, but Looks Good To Me.
Thanks Roger. Here is an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.01 best regards, -- daniel On 08/06/16 15:56, Roger Riggs wrote:
+1
On 6/8/2016 10:37 AM, Daniel Fuchs wrote:
On 08/06/16 15:35, Roger Riggs wrote:
Yes, I would update the spec to the new behavior.
What about:
* <li>Otherwise, if the string contains {@code "{<digit>"} * where {@code <digit>} is in {@code [0-9]} * then java.text.MessageFormat is used to format the string. (The current phrasing does not use 'then').
Hi Daniel, Thanks for taking this on. Looks good. This might be a new issue altogether but, if record.getMessage returns null a NPE can be generated during 'catalog.getString' (per RB.handleGetObject contract) or 'java.text.MessageFormat.format' (undocumented) It is handled by the catch clause so it is harmless in terms of correctness. The most common form of this abuse I've seen in the wild is 'logger.log(SEVERE, null, (Throwable) ioe); So if it not worth explicitly checking for null maybe we should add a comment explaining the intent that NPE could be generated and is trapped. For sure having tests for null message with and without a resource bundle would be a good idea. Thanks, Jason From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Daniel Fuchs <daniel.fuchs@oracle.com> Sent: Wednesday, June 8, 2016 8:46 AM To: core-libs-dev Subject: RFR: 8153666: Optimize Formatter.formatMessage Hi, Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless. More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-... (thanks Jason!) bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666 patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00 best regards, and thanks to all who provided suggestions and archeological background! -- daniel
Hi Jason, On 08/06/16 15:31, Jason Mehrens wrote:
Hi Daniel,
Thanks for taking this on. Looks good.
This might be a new issue altogether but, if record.getMessage returns null a NPE can be generated during 'catalog.getString' (per RB.handleGetObject contract) or 'java.text.MessageFormat.format' (undocumented) It is handled by the catch clause so it is harmless in terms of correctness. The most common form of this abuse I've seen in the wild is 'logger.log(SEVERE, null, (Throwable) ioe);
So if it not worth explicitly checking for null maybe we should add a comment explaining the intent that NPE could be generated and is trapped. For sure having tests for null message with and without a resource bundle would be a good idea.
Thanks again for pointing that out. I thought we already had tests for that but it appear we don't. The package.html says: << In general, unless otherwise noted in the javadoc, methods and constructors will throw NullPointerException if passed a null argument. The one broad exception to this rule is that the logging convenience methods in the Logger class (the config, entering, exiting, fine, finer, finest, log, logp, logrb, severe, throwing, and warning methods) will accept null values for all arguments except for the initial Level argument (if any).
I am afraid this is both not true and not tested (sigh). The new methods that accept a Supplier<String> may throw a NPE for instance if the supplier is null and the level is enabled. So it looks that we need to make yet another decision about how to bring the spec and implementation in sync here, and we definitely need more tests. I will log a new issue for this - as as you say it appears to have little to do with what this patch is about. best regards, -- daniel
Thanks,
Jason
From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Daniel Fuchs <daniel.fuchs@oracle.com> Sent: Wednesday, June 8, 2016 8:46 AM To: core-libs-dev Subject: RFR: 8153666: Optimize Formatter.formatMessage
Hi,
Please find below a patch for a small optimization in Formatter.formatMessage. This patch also removed the synchronized keyword which was there for historical reasons - but which has become needless.
More background available at: https://bugs.openjdk.java.net/browse/JDK-8153666 (thanks Martin!) and http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-... (thanks Jason!)
bug: 8153666: Optimize Formatter.formatMessage https://bugs.openjdk.java.net/browse/JDK-8153666
patch: http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00
best regards, and thanks to all who provided suggestions and archeological background!
-- daniel
Hi Jason, On 08/06/16 16:39, Daniel Fuchs wrote:
I will log a new issue for this - as as you say it appears to have little to do with what this patch is about.
I have logged 8159070: Null pointer handling in java.util.logging.Logger is under tested and may not always conform to specification https://bugs.openjdk.java.net/browse/JDK-8159070 There may be tests that verify that you can pass null in the JCK - I haven't investigated those - but it won't hurt adding some new unit tests anyway. best regards, -- daniel
participants (4)
-
Daniel Fuchs
-
Jason Mehrens
-
Martin Buchholz
-
Roger Riggs