original) (raw)
(Dear all,
I just realized there is another problem with PlatformLogger log statements:
XBaseWindow:
��� public boolean grabInput() {
������� grabLog.fine("Grab input on {0}", this);
...
}
This calls the PlatformLogger.fine( varargs):
��� public void fine(String msg, Object... params) {
������� logger.doLog(FINE, msg, params);
��� }
Doing so, the JVM creates a new Object[] instance to provide params as varargs.
I would recommend using isLoggable() test to avoid such waste if the log is disabled (fine, finer, finest ...).
Do you want me to provide a new patch related to this problem ?
Does somebody have an idea to automatically analyze the JDK code and detect missing isLoggable statements ...
Regards,
Laurent
Anthony,
could you tell me once it is in the OpenJDK8 repository ?
I would then perform again profiling tests to check if there is no more missing isLoggable() statements.
Once JMX and other projects switch to PlatformLogger, I could check again.
Maybe I could write a small java code checker (pmd rule) to test if there is missing isLoggable() statements wrapping PlatformLogger log statements. Any idea about how to reuse java parser to do so ?
Regards,
Laurent2013/4/2 Anthony Petrov <anthony.petrov@oracle.com>Looks fine to me as well. Thanks for fixing this, Laurent.
Let's wait a couple more days in case Net or Swing folks want to review the fix. After that I'll push it to the repository.
--
best regards,
Anthony
On 4/2/2013 15:35, Laurent Bourg�s wrote:
Here is the updated patch:2013/4/2 Anthony Petrov <anthony.petrov@oracle.com <mailto:anthony.petrov@oracle.com>>
http://jmmc.fr/\~bourgesl/share/webrev-8010297.2/
Fixed inconsistencies between FINE / FINER log statements:
\- XScrollbarPeer
\- XWindowPeer
Laurent
� � � � + � � � �if (log.isLoggable(\_\_PlatformLogger.FINEST)) {
� � 1\. Sergey: I believe this is for purposes of better formating the
� � log output and making it more readable by separating or highlighting
� � some sections. I don't think this should be changed.
� � 2\. Laurent: can you please address this issue and send us a new patch?
� � --
� � best regards,
� � Anthony
� � On 4/1/2013 16:08, Sergey Bylokhov wrote:
� � � � Hi, Anthony
� � � � Only two comments:
� � � � 1 Why we need some special text in the log output like "\*\*\*" and
� � � � "###"
� � � � 2 XScrollbarPeer.java:
� � � � + � � � � � �log.finer("KeyEvent on scrollbar: " + event);
� � � � + � � � �}
� � � � On 4/1/13 12:20 PM, Anthony Petrov wrote:
� � � � � � Awt, Swing, Net engineers,
� � � � � � Could anyone review the fix please? For your convenience:
� � � � � � Bug: http://bugs.sun.com/view\_bug.\_\_do?bug\_id=8010297
� � � � � � <http://bugs.sun.com/view\_bug.do?bug\_id=8010297>
� � � � � � Fix:
� � � � � � http://cr.openjdk.java.net/\~\_\_anthony/8-55-isLoggable-\_\_8010297.0/
� � � � � � <http://cr.openjdk.java.net/%7Eanthony/8-55-isLoggable-8010297.0/>
� � � � � � -- � � � � � � best regards,� � � � � � � � � � http://bugs.sun.com/view\_bug.\_\_do?bug\_id=8010297
� � � � � � Anthony
� � � � � � On 3/22/2013 2:26, Anthony Petrov wrote:
� � � � � � � � Hi Laurent,
� � � � � � � � The fix looks great to me. Thank you very much.
� � � � � � � � We still need at least one review, though. Hopefully
� � � � � � � � net-dev@ and/or swing-dev@ folks might help us out a bit.
� � � � � � � � -- � � � � � � � � best regards,
� � � � � � � � Anthony
� � � � � � � � On 3/20/2013 15:10, Anthony Petrov wrote:
� � � � � � � � � � Hi Laurent,
� � � � � � � � � � Thanks for the patch. I've filed a bug at:� � � � � � � � � � http://cr.openjdk.java.net/\~\_\_anthony/8-55-isLoggable-\_\_8010297.0/
� � � � � � � � � � <http://bugs.sun.com/view\_bug.do?bug\_id=8010297>
� � � � � � � � � � (should be available in a day or two)
� � � � � � � � � � and published a webrev generated from your patch at:
� � � � � � � � � � <http://cr.openjdk.java.net/%7Eanthony/8-55-isLoggable-8010297.0/>
� � � � � � � � � � I'm also copying swing-dev@ and net-dev@ because the
� � � � � � � � � � fix affects those areas too. I myself will review
� � � � � � � � � � the fix a bit later but am sending it now for other
� � � � � � � � � � folks to take a look at it.
� � � � � � � � � � On 3/19/2013 15:29, Laurent Bourg�s wrote:
� � � � � � � � � � � � I am sorry I started modifying PlatformLogger
� � � � � � � � � � � � and reverted changes to this class as it is
� � � � � � � � � � � � another topic to be discussed later: isLoggable
� � � � � � � � � � � � performance and waste due to HashMap<Integer,
� � � � � � � � � � � � Level> leads to Integer allocations (boxing).
� � � � � � � � � � I saw your message to core-libs-dev@, so I just
� � � � � � � � � � dropped all changes to the PlatformLogger from this
� � � � � � � � � � patch.
� � � � � � � � � � � � Finally, I have another question related to the
� � � � � � � � � � � � WrapperGenerator class: it generates a lot of
� � � � � � � � � � � � empty log statements (XEvent):
� � � � � � � � � � � � �log� � � � � � � � � � � � <http://grepcode.com/file/\_\_repository.grepcode.com/java/\_\_root/jdk/openjdk/6-b14/sun/\_\_awt/X11/XWrapperBase.java#\_\_XWrapperBase.0log
� � � � � � � � � � � � <http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/awt/X11/XWrapperBase.java#XWrapperBase.0log>>.finest
� � � � � � � � � � � � <http://grepcode.com/file/__repository.grepcode.com/java/__root/jdk/openjdk/6-b14/java/__util/logging/Logger.java#__Logger.finest%28java.lang.__String%29
� � � � � � � � � � � � <http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/logging/Logger.java#Logger.finest%28java.lang.String%29>>("");� � � � � � � � � � � � (generateLog?"log.finest(\\"\\")\_\_;":"");
� � � � � � � � � � � � Is it really useful to have such statements ? I
� � � � � � � � � � � � would keep logs with non empty messages only.
� � � � � � � � � � � � See WrapperGenerator:753:
� � � � � � � � � � � � � � � � String s\_log =� � � � � � � � � � � � <mailto:bourges.laurent@gmail.\_\_com
� � � � � � � � � � I believe they're used for log formatting purposes
� � � � � � � � � � to separate numerous events in a log (e.g. think of
� � � � � � � � � � mouse-move events - there can be hundreds of them in
� � � � � � � � � � a raw).
� � � � � � � � � � Please note that the hg export format is not that
� � � � � � � � � � useful unless you're assigned an OpenJDK id already
� � � � � � � � � � (please see Dalibor's message for details) because I
� � � � � � � � � � can't import it directly. So for the time being you
� � � � � � � � � � could send just raw patches (i.e. the output of hg
� � � � � � � � � � diff only - and there's no need to commit your
� � � � � � � � � � changes in this case). Also, please note that the
� � � � � � � � � � mailing lists strip attachments. The reason I got it
� � � � � � � � � � is because I was listed in To: of your message. So
� � � � � � � � � � when sending patches you can:
� � � � � � � � � � 1) post them inline, or
� � � � � � � � � � 2) attach them and add a person to To: of your
� � � � � � � � � � message, or
� � � � � � � � � � 3) upload them somewhere on the web.
� � � � � � � � � � However, it would be best if you could generate a
� � � � � � � � � � webrev for your changes and upload it somewhere.
� � � � � � � � � � Currently this is the standard format for reviewing
� � � � � � � � � � fixes in OpenJDK.
� � � � � � � � � � -- � � � � � � � � � � best regards,
� � � � � � � � � � Anthony
� � � � � � � � � � � � Regards,
� � � � � � � � � � � � Laurent
� � � � � � � � � � � � 2013/3/19 Laurent Bourg�s
� � � � � � � � � � � � <bourges.laurent@gmail.com
� � � � � � � � � � � � <mailto:bourges.laurent@gmail.com>� � � � � � � � � � � � � � if (...log.isLoggable(\_\_PlatformLogger.xxx)) {
� � � � � � � � � � � � <mailto:bourges.laurent@gmail.com>>>
� � � � � � � � � � � � � � Hi antony,
� � � � � � � � � � � � � � FYI I started reviewing and fixing all
� � � � � � � � � � � � PlatformLogger use cases (not
� � � � � � � � � � � � � � too many as I thought first) mainly used by
� � � � � � � � � � � � awt / swing projects to
� � � � � � � � � � � � � � provide you a patch on latest JDK8 source code:
� � � � � � � � � � � � � � I am adding the log level check when it is
� � � � � � � � � � � � missing:� � � � � � � � � � � � � � <mailto:bourges.laurent@gmail.\_\_com
� � � � � � � � � � � � � � � � log...
� � � � � � � � � � � � � � }
� � � � � � � � � � � � � � I will not change the String + operations to
� � � � � � � � � � � � use the message format
� � � � � � � � � � � � � � syntax in this patch.
� � � � � � � � � � � � � � Do you accept such patch / proposed
� � � � � � � � � � � � contribution ?
� � � � � � � � � � � � � � Regards,
� � � � � � � � � � � � � � Laurent
� � � � � � � � � � � � � � 2013/3/18 Laurent Bourg�s
� � � � � � � � � � � � <bourges.laurent@gmail.com
� � � � � � � � � � � � <mailto:bourges.laurent@gmail.com>� � � � � � � � � � � � � � � � � � � � � � � � � � � �http://mail.openjdk.java.net/\_\_pipermail/jdk7u-dev/2012-\_\_April/002751.html
� � � � � � � � � � � � <mailto:bourges.laurent@gmail.com>>>
� � � � � � � � � � � � � � � � Hi antony,
� � � � � � � � � � � � � � � � 2 different things:
� � � � � � � � � � � � � � � � 1/ PlatformLogger was patched (doLog
� � � � � � � � � � � � method) to avoid string
� � � � � � � � � � � � � � � � operations (message formatting) for
� � � � � � � � � � � � disabled logs (patch
� � � � � � � � � � � � � � � � submiited on JDK8 and JDK7u):� � � � � � � � � � � � � � � � (log.isLoggable(\_\_PlatformLogger.FINE))
� � � � � � � � � � � � <http://mail.openjdk.java.net/pipermail/jdk7u-dev/2012-April/002751.html>
� � � � � � � � � � � � � � � � 2/ I looked 2 hours ago on JDK7u AND
� � � � � � � � � � � � JDK8 source codes and both
� � � � � � � � � � � � � � � � still have:
� � � � � � � � � � � � � � � � - log statements WITHOUT log level check
� � � � � � � � � � � � : if� � � � � � � � � � � � � � � � <mailto:anthony.petrov@oracle.\_\_com
� � � � � � � � � � � � log.fine(...);
� � � � � � � � � � � � � � � � - string operations (+) in log calls
� � � � � � � � � � � � that could be improved
� � � � � � � � � � � � � � � � using the message format syntax (String
� � � � � � � � � � � � + args): for example,
� � � � � � � � � � � � � � � � avoid using PlatformLogger.fine(String +
� � � � � � � � � � � � ...) in favor of using
� � � � � � � � � � � � � � � � PlatformLogger.fine(String msg,
� � � � � � � � � � � � Object... params)
� � � � � � � � � � � � � � � � I reported in my previous mail several
� � � � � � � � � � � � cases where the
� � � � � � � � � � � � � � � � isLoggable() call is missing and leads
� � � � � � � � � � � � to useless String
� � � � � � � � � � � � � � � � operations but also method calls
� � � � � � � � � � � � (Component.paramString() for
� � � � � � � � � � � � � � � � example).
� � � � � � � � � � � � � � � � Finally, I also provided other possible
� � � � � � � � � � � � cases (using grep);
� � � � � � � � � � � � � � � � maybe there is a better alternative to
� � � � � � � � � � � � find all occurences of
� � � � � � � � � � � � � � � � String operations in log calls.
� � � � � � � � � � � � � � � � Regards,
� � � � � � � � � � � � � � � � Laurent
� � � � � � � � � � � � � � � � 2013/3/18 Anthony Petrov
� � � � � � � � � � � � <anthony.petrov@oracle.com
� � � � � � � � � � � � <mailto:anthony.petrov@oracle.com>� � � � � � � � � � � � � � � � � � � � \* � � � � � � � � � � � � � � � stateLog.finer("\_\_\_\_doStateProtocol() returns " +
� � � � � � � � � � � � <mailto:anthony.petrov@oracle.com>>>
� � � � � � � � � � � � � � � � � � Hi Laurent,
� � � � � � � � � � � � � � � � � � Normally we fix an issue in JDK 8
� � � � � � � � � � � � first, and then back-port
� � � � � � � � � � � � � � � � � � the fix to a 7u release. You're
� � � � � � � � � � � � saying that in JDK 8 the
� � � � � � � � � � � � � � � � � � problem isn't reproducible anymore.
� � � � � � � � � � � � Can you please
� � � � � � � � � � � � � � � � � � investigate (using the Mercurial
� � � � � � � � � � � � history log) what exact fix
� � � � � � � � � � � � � � � � � � resolved it in JDK 8?
� � � � � � � � � � � � � � � � � � --
� � � � � � � � � � � � � � � � � � best regards,
� � � � � � � � � � � � � � � � � � Anthony
� � � � � � � � � � � � � � � � � � On 03/18/13 15:09, Laurent Bourg�s
� � � � � � � � � � � � wrote:
� � � � � � � � � � � � � � � � � � � � Dear all,
� � � � � � � � � � � � � � � � � � � � I run recently netbeans profiler
� � � � � � � � � � � � on my swing application
� � � � � � � � � � � � � � � � � � � � (Aspro2:
� � � � � � � � � � � � � � � � � � � � http://www.jmmc.fr/aspro) under
� � � � � � � � � � � � linux x64 platform and I
� � � � � � � � � � � � � � � � � � � � figured out
� � � � � � � � � � � � � � � � � � � � that a lot of char\[\] instances
� � � � � � � � � � � � are coming from String +
� � � � � � � � � � � � � � � � � � � � operator called
� � � � � � � � � � � � � � � � � � � � by sun.awt.X11 code.
� � � � � � � � � � � � � � � � � � � � I looked at PlatformLogger
� � � � � � � � � � � � source code but found not way
� � � � � � � � � � � � � � � � � � � � to disable it
� � � � � � � � � � � � � � � � � � � � completely: maybe an empty
� � � � � � � � � � � � logger implementation could
� � � � � � � � � � � � � � � � � � � � be interesting to
� � � � � � � � � � � � � � � � � � � � be used during profiling or
� � � � � � � � � � � � normal use (not debugging).
� � � � � � � � � � � � � � � � � � � � Apparently JDK8 provides some
� � � � � � � � � � � � patchs to avoid String
� � � � � � � � � � � � � � � � � � � � creation when the
� � � � � � � � � � � � � � � � � � � � logger is disabled (level).
� � � � � � � � � � � � � � � � � � � � However, I looked also to the
� � � � � � � � � � � � sun.awt code (jdk7u
� � � � � � � � � � � � � � � � � � � � repository) to see the
� � � � � � � � � � � � � � � � � � � � origin of the string allocations:
� � � � � � � � � � � � � � � � � � � � XDecoratedPeer:
� � � � � � � � � � � � � � � � � � � � � � �public void
� � � � � � � � � � � � handleFocusEvent(XEvent xev) {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � \* � � � � � � � � � � � � � � � focusLog.finer("Received focus event on shell:
� � � � � � � � � � � � � � � � � � � � " + xfe);
� � � � � � � � � � � � � � � � � � � � \* � }
� � � � � � � � � � � � � � � � � � � � � � �public boolean
� � � � � � � � � � � � requestWindowFocus(long time,
� � � � � � � � � � � � � � � � � � � � boolean timeProvided) {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � \* � � � � � � � � � � � � � � � � � focusLog.finest("Real native focused
� � � � � � � � � � � � � � � � � � � � window: " +
� � � � � � � � � � � � � � � � � � � � realNativeFocusedWindow +
� � � � � � � � � � � � � � � � � � � � "\\nKFM's focused window: " +
� � � � � � � � � � � � focusedWindow);
� � � � � � � � � � � � � � � � � � � � \*...
� � � � � � � � � � � � � � � � � � � � \* � � � � � � � � � � � � � � � focusLog.fine("Requesting focus to " + (this ==
� � � � � � � � � � � � � � � � � � � � toFocus ? "this
� � � � � � � � � � � � � � � � � � � � window" : toFocus));
� � � � � � � � � � � � � � � � � � � � \*...
� � � � � � � � � � � � � � � � � � � � }
� � � � � � � � � � � � � � � � � � � � XBaseWindow:
� � � � � � � � � � � � � � � � � � � � � � �public void xSetBounds(int
� � � � � � � � � � � � x, int y, int width, int
� � � � � � � � � � � � � � � � � � � � height) {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � \* � � � �insLog.fine("Setting
� � � � � � � � � � � � bounds on " + this + " to
� � � � � � � � � � � � � � � � � � � � (" + x + ", " +
� � � � � � � � � � � � � � � � � � � � y + "), " + width + "x" + height);
� � � � � � � � � � � � � � � � � � � � \*}
� � � � � � � � � � � � � � � � � � � � XNetProtocol:
� � � � � � � � � � � � � � � � � � � � � � �boolean doStateProtocol() {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � res);
� � � � � � � � � � � � � � � � � � � � \*}
� � � � � � � � � � � � � � � � � � � � XSystemTrayPeer:
� � � � � � � � � � � � � � � � � � � � � � �XSystemTrayPeer(SystemTray
� � � � � � � � � � � � target) {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � \* � � � �log.fine(" check if
� � � � � � � � � � � � system tray is available.
� � � � � � � � � � � � � � � � � � � � selection owner:
� � � � � � � � � � � � � � � � � � � � " + selection\_owner);
� � � � � � � � � � � � � � � � � � � � \*}
� � � � � � � � � � � � � � � � � � � � � � �void
� � � � � � � � � � � � addTrayIcon(XTrayIconPeer tiPeer) throws
� � � � � � � � � � � � � � � � � � � � AWTException {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � \* � � � �log.fine(" send
� � � � � � � � � � � � SYSTEM\_TRAY\_REQUEST\_DOCK
� � � � � � � � � � � � � � � � � � � � message to owner: " +
� � � � � � � � � � � � � � � � � � � � selection\_owner);
� � � � � � � � � � � � � � � � � � � � \*}
� � � � � � � � � � � � � � � � � � � � XFramePeer:
� � � � � � � � � � � � � � � � � � � � � � �public void
� � � � � � � � � � � � handlePropertyNotify(XEvent xev) {
� � � � � � � � � � � � � � � � � � � � ...
� � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � stateLog.finer("State is the same: " + state);
� � � � � � � � � � � � � � � � � � � � }
� � � � � � � � � � � � � � � � � � � � However I only give here few
� � � � � � � � � � � � cases but certainly others
� � � � � � � � � � � � � � � � � � � � are still
� � � � � � � � � � � � � � � � � � � � present in the source code;
� � � � � � � � � � � � maybe findbugs or netbeans
� � � � � � � � � � � � � � � � � � � � warnings could
� � � � � � � � � � � � � � � � � � � � help you finding all of them.
� � � � � � � � � � � � � � � � � � � � I advocate the amount of waste
� � � � � � � � � � � � (GC) is not very
� � � � � � � � � � � � � � � � � � � � important but String
� � � � � � � � � � � � � � � � � � � � conversion are also calling
� � � � � � � � � � � � several toString() methods
� � � � � � � � � � � � � � � � � � � � that can be
� � � � � � � � � � � � � � � � � � � � costly (event, Frame, window ...)
� � � � � � � � � � � � � � � � � � � � Finally, I ran few grep commands
� � � � � � � � � � � � on the sun.awt.X11 code
� � � � � � � � � � � � � � � � � � � � (jdk7u) and you
� � � � � � � � � � � � � � � � � � � � can look at them to see all
� � � � � � � � � � � � string + operations related
� � � � � � � � � � � � � � � � � � � � to log statements.
� � � � � � � � � � � � � � � � � � � � PS: I may help fixing the source
� � � � � � � � � � � � code but I have no idea
� � � � � � � � � � � � � � � � � � � � how to
� � � � � � � � � � � � � � � � � � � � collaborate (provide a patch ?)
� � � � � � � � � � � � � � � � � � � � Regards,
� � � � � � � � � � � � � � � � � � � � Laurent Bourg�s
� � � � -- � � � � Best regards, Sergey.