Here is the updated patch:
http://jmmc.fr/~bourgesl/share/webrev-8010297.2/
Fixed inconsistencies between FINE / FINER log statements:
- XScrollbarPeer
- XWindowPeer
Laurent
2013/4/2 Anthony Petrov <anthony.pet...@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<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<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<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>>("");
>>>>>>
>>>>>>
>>>>>> 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.laur...@gmail.com <mailto:
>>>>>> bourges.laurent@gmail.**com <bourges.laur...@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.laur...@gmail.com
>>>>>> <mailto:bourges.laurent@gmail.**com <bourges.laur...@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<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.pet...@oracle.com
>>>>>> <mailto:anthony.petrov@oracle.**com<anthony.pet...@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.
>>
>
