<Swing Dev> <AWT Dev> sun.awt.X11 logs still using String + (waste)
Anthony Petrov
anthony.petrov at oracle.com
Wed Mar 20 11:10:07 UTC 2013
Hi Laurent,
Thanks for the patch. I've filed a bug at:
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/~anthony/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>.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>("");
>
> Is it really useful to have such statements ? I would keep logs with non
> empty messages only.
>
> See WrapperGenerator:753:
> String s_log = (generateLog?"log.finest(\"\");":"");
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 at gmail.com
> <mailto:bourges.laurent at 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:
> if (...log.isLoggable(PlatformLogger.xxx)) {
> 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 at gmail.com
> <mailto:bourges.laurent at 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):
> 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
> (log.isLoggable(PlatformLogger.FINE)) 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 at oracle.com
> <mailto:anthony.petrov at 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() {
> ...
> * stateLog.finer("__doStateProtocol() returns " +
> 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
>
>
>
>
More information about the swing-dev
mailing list