<Swing Dev> <AWT Dev> sun.awt.X11 logs still using String + (waste)
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Tue Apr 2 12:03:44 UTC 2013
Hi, Laurent.
Fix looks good.
Thanks!
On 4/2/13 3:35 PM, Laurent Bourgès wrote:
> Here is the updated patch:
> http://jmmc.fr/~bourgesl/share/webrev-8010297.2/
> <http://jmmc.fr/%7Ebourgesl/share/webrev-8010297.2/>
>
> Fixed inconsistencies between FINE / FINER log statements:
> - XScrollbarPeer
> - XWindowPeer
>
> Laurent
>
> 2013/4/2 Anthony Petrov <anthony.petrov at oracle.com
> <mailto:anthony.petrov at oracle.com>>
>
> 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:
>
> + if (log.isLoggable(PlatformLogger.FINEST)) {
> + 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
>
> 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,
> 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://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/
> <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>.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>
> <mailto: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>
> <mailto: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>
> <mailto: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
>
>
>
>
>
>
> --
> Best regards, Sergey.
>
>
--
Best regards, Sergey.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20130402/3184d2db/attachment.html>
More information about the swing-dev
mailing list