Hi Laurent,
Thanks for all your hard work! The fix looks fine to me.
I'm also CC'ing jdk8-dev@ to let other teams review the changes in their
areas. I've only reviewed the AWT part of the fix.
--
best regards,
Anthony
On 4/8/2013 15:32, Laurent Bourgès wrote:
> Anthony,
>
> here is an updated patch concerning both PlatformLogger and Logger 'bad'
> usages:
> http://jmmc.fr/~bourgesl/share/webrev-8010297.3/
>
> It impacts now awt, swing, JMX, security, network, and apache xml.
>
> Thanks for the review,
> Laurent
>
> 2013/4/8 Laurent Bourgès <bourges.laurent at gmail.com
> <mailto:bourges.laurent at gmail.com>>
>
> Peter, Mandy,
>
> I think it would be great to make PlatformLogger very similar to
> Logger API:
> I mean JUL Logger already has only 1 convenient method with 1 param
> so it may be great to have the same API in PlatformLogger too: maybe
> extend it to 2 or 3 params ...
>
> Peter, could you look at the API differences between Logger /
> PlatformLogger to make PlatformLogger API more similar to JUL Logger ?
>
> Laurent
>
>
> 2013/4/8 Peter Levart <peter.levart at gmail.com
> <mailto:peter.levart at gmail.com>>
>
> On 04/07/2013 07:11 PM, Laurent Bourgès wrote:
>>
>> Hi
>>
>> I started fixing All PlatformlLogger "bad" usages and I am
>> fixing also many jul Logger usages without isLoggable ...
>> That represents a lot of changes and a very boring job.
>>
>> I think sending a webrew tomorrow.
>>
>
> Hi Laurent,
>
> Since you're already deep in the task, you might have a rough
> feeling what part of such unguarded log statements are of the
> following type:
>
> logger.[fine,info,...]("a message with {0} and {1}
> placeholders", someArg1, someArg2);
>
> where someArg1, ... are some values that are already present in
> the context of the logging statement and don't have to be
> computed just for satisfying the logging (not even boxing). Such
> usages could be effectively optimized by adding some overloaded
> methods to PlatformLogger that take 1, 2, 3, ... arguments:
>
> class PlatformLogger {
>
> ...
>
> public void finest(String msg, Object param1) {
> if (isLoggable(Level.FINEST)) {
> loggerProxy.doLog(Level.FINEST, msg, param1);
> }
> }
>
> public void finest(String msg, Object param1, Object param2) {
> if (isLoggable(Level.FINEST)) {
> loggerProxy.doLog(Level.FINEST, msg, param1, param2);
> }
> }
>
> public void finest(String msg, Object param1, Object param2,
> Object param3) {
> if (isLoggable(Level.FINEST)) {
> loggerProxy.doLog(Level.FINEST, msg, param1, param2,
> param3);
> }
> }
>
> ...
>
> This would effectively guard creation of the arguments array
> with an isLoggable check for some common usages, eliminating the
> need to explicitly guard such statements. Of course, the user
> would have to be aware of when such unguarded logging statement
> is without overhead and when not...
>
> How do you feel about such API extension?
>
>
> Regards, Peter
>
>
>
>> Laurent
>>
>> Le 4 avr. 2013 14:08, "Laurent Bourgès"
>> <bourges.laurent at gmail.com <mailto:bourges.laurent at gmail.com>>
>> a écrit :
>>
>> Ok, I can provide an updated patch after finding / fixing
>> all usages.
>>
>> Probably in 1 or 2 days,
>>
>> Laurent
>>
>> 2013/4/4 Anthony Petrov <anthony.petrov at oracle.com
>> <mailto:anthony.petrov at oracle.com>>
>>
>> Yes, this makes sense. Do you want to update your fix
>> for 8010297 to include these changes as well?
>>
>> --
>> best regards,
>> Anthony
>>
>>
>> On 04/04/13 15:47, Laurent Bourgès wrote:
>>
>> 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
>>
>> 2013/4/3 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>>>
>>
>>
>> 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,
>>
>> Laurent
>>
>> 2013/4/2 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>>>
>>
>>
>> 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:
>>
>> http://jmmc.fr/~bourgesl/__share/webrev-8010297.2/
>> <http://jmmc.fr/%7Ebourgesl/__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>
>> <mailto:anthony.petrov at oracle.com
>> <mailto:anthony.petrov at oracle.com>>
>> <mailto:anthony.petrov at oracle.
>> <mailto:anthony.petrov at oracle.>__com
>>
>> <mailto: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
>>
>> <http://bugs.sun.com/view_bug.__do?bug_id=8010297>
>>
>>
>> <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/%7E____anthony/8-55-isLoggable-____8010297.0/>
>>
>> <http://cr.openjdk.java.net/%7E__anthony/8-55-isLoggable-__8010297.0/>
>>
>> <http://cr.openjdk.java.net/%__7Eanthony/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>
>>
>>
>>
>> <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/%7E____anthony/8-55-isLoggable-____8010297.0/>
>>
>> <http://cr.openjdk.java.net/%7E__anthony/8-55-isLoggable-__8010297.0/>
>>
>> <http://cr.openjdk.java.net/%__7Eanthony/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>
>>
>>
>>
>> <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>
>>
>>
>>
>> <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.laurent at gmail.com
>> <mailto:bourges.laurent at gmail.com>
>> <mailto:bourges.laurent at gmail.com
>> <mailto:bourges.laurent at gmail.com>>
>> <mailto:bourges.laurent at gmail.
>> <mailto:bourges.laurent at gmail.>__com
>> <mailto:bourges.laurent at gmail.com
>> <mailto:bourges.laurent at gmail.com>>>
>> <mailto:bourges.laurent at gmail
>> <mailto:bourges.laurent at gmail>.
>> <mailto:bourges.laurent at gmail
>> <mailto:bourges.laurent at gmail>.>____com
>>
>> <mailto:bourges.laurent at gmail.
>> <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>>
>> <mailto:bourges.laurent at gmail.
>> <mailto:bourges.laurent at gmail.>__com
>> <mailto:bourges.laurent at gmail.com
>> <mailto:bourges.laurent at gmail.com>>>
>> <mailto:bourges.laurent at gmail
>> <mailto:bourges.laurent at gmail>.
>> <mailto:bourges.laurent at gmail
>> <mailto:bourges.laurent at gmail>.>____com
>>
>> <mailto:bourges.laurent at gmail.
>> <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
>>
>> <http://mail.openjdk.java.net/__pipermail/jdk7u-dev/2012-__April/002751.html>
>>
>>
>>
>>
>> <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.petrov at oracle.com
>> <mailto:anthony.petrov at oracle.com>
>> <mailto:anthony.petrov at oracle.com
>> <mailto:anthony.petrov at oracle.com>>
>> <mailto:anthony.petrov at oracle.
>> <mailto:anthony.petrov at oracle.>__com
>> <mailto:anthony.petrov at oracle.com
>> <mailto:anthony.petrov at oracle.com>>>
>> <mailto:anthony.petrov at oracle
>> <mailto:anthony.petrov at oracle>.
>> <mailto:anthony.petrov at oracle
>> <mailto:anthony.petrov at oracle>.>____com
>>
>> <mailto:anthony.petrov at oracle.
>> <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.
>>
>>
>>
>>
>>
>
>
>