Yes.
Good observation!
Although JDK 6 does have a great many optimizations for locking, i.e.
biased locking, for uncontended locks (which we have here),
StringBuilder would definitely be a better choice. We should keep the
"bear" as fast as we can make 'em even we he's leaving a logging trail. ;-)
charlie ...
PS: I've added 'dev' to my response since not all 'committers' may not
be subscribed to 'commits'.
kedar mhaswade wrote:
> Charlie,
>
> Great analysis and suggestions, but but ... since this method
> allocates a new variable sb which is stack-confined, shouldn't it be
> an instance of StringBuilder instead of StringBuffer?
>
> -Kedar
>
> On Wed, Oct 29, 2008 at 7:11 AM, charlie hunt <charlie.hunt_at_sun.com
> <mailto:charlie.hunt_at_sun.com>> wrote:
>
> Should we consider a small enhancement to this commit?
>
> How about explicitly sizing the new StringBuffer in
> format(LogRecord record)?
>
> As you know, new StringBuffer() allocates an underlying char[] of
> size 16. Adding to that, StringBuffer.append(String str), which
> is used quite often in format(LogRecord record), does a resize of
> the underlying char[] based on the incoming String's length + the
> underlying char's count. So, if the existing StringBuffer is
> sized at 16, and all 16 char's are used, and we do a
> StringBuffer.append("]"), we throw away the old underlying char[]
> at size 16, and re-size it to char[] size 17.
>
> In quickly scanning through format(LogRecord record), there may be
> quite a few opportunities to be re-allocating StringBuffer char[]'s.
>
> From the API comments for format(LogRecord record), it looks like
> there's about 90 or so character's with having a rather small
> "message". Perhaps sizing the StringBuffer to 128 may be more
> appropriate?
>
> Alternatively, one could put the StringBuffer in a ThreadLocal and
> do a StringBuffer.setLength(0) after grabbing it from the
> ThreadLocal and before using it. This, of course, has the down
> side of potentially having a large number of StringBuffer's (i.e.
> large number of Threads) living in the application for an extended
> period of time.
>
> But, I don't think I would take this alternative approach unless
> we observed this method being a hotspot wrt large number of char[]
> re-allocations. In addition, the pattern of a 'new StringBuffer'
> and StringBuffer.toString() operation being done in the same
> method in more recent JDK 6 update releases will be optimized via
> escape analysis to elide the char[] copy to a new String that's
> done in StringBuffer.toString() since the underlying char[] in
> StringBuffer does not escape the method. So, if we did a good job
> at sizing the initial StringBuffer, we could get very close to
> eliminating any "unnecessary" char[] allocation.
>
> charlie ...
>
>
> survivant_at_dev.java.net <mailto:survivant_at_dev.java.net> wrote:
>
> Author: survivant
> Date: 2008-10-29 13:27:58+0000
> New Revision: 1688
>
> Added:
>
> trunk/modules/grizzly/src/main/java/com/sun/grizzly/util/LoggingFormatter.java
>
> Log:
> A logger that include the Thread, that logged the event, in
> the log.
> a sample of the output
> [main] Wed Oct 29 09:24:56 EDT 2008 [SEVERE]
> com.sun.grizzly.util.LoggingFormatter:main Severe Event
>
> Added:
> trunk/modules/grizzly/src/main/java/com/sun/grizzly/util/LoggingFormatter.java
> Url:
> https://grizzly.dev.java.net/source/browse/grizzly/trunk/modules/grizzly/src/main/java/com/sun/grizzly/util/LoggingFormatter.java?view=auto&rev=1688
> <https://grizzly.dev.java.net/source/browse/grizzly/trunk/modules/grizzly/src/main/java/com/sun/grizzly/util/LoggingFormatter.java?view=auto&rev=1688>
> ==============================================================================
> --- (empty file)
> +++
> trunk/modules/grizzly/src/main/java/com/sun/grizzly/util/LoggingFormatter.java
> 2008-10-29 13:27:58+0000
> @@ -0,0 +1,121 @@
> +/*
> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
> + *
> + * Copyright 1997-2007 Sun Microsystems, Inc. All rights
> reserved.
> + *
> + * The contents of this file are subject to the terms of
> either the GNU
> + * General Public License Version 2 only ("GPL") or the
> Common Development
> + * and Distribution License("CDDL") (collectively, the
> "License"). You
> + * may not use this file except in compliance with the
> License. You can obtain
> + * a copy of the License at
> https://glassfish.dev.java.net/public/CDDL+GPL.html
> + * or glassfish/bootstrap/legal/LICENSE.txt. See the License
> for the specific
> + * language governing permissions and limitations under the
> License.
> + *
> + * When distributing the software, include this License
> Header Notice in each
> + * file and include the License file at
> glassfish/bootstrap/legal/LICENSE.txt.
> + * Sun designates this particular file as subject to the
> "Classpath" exception
> + * as provided by Sun in the GPL Version 2 section of the
> License file that
> + * accompanied this code. If applicable, add the following
> below the License
> + * Header, with the fields enclosed by brackets [] replaced
> by your own
> + * identifying information: "Portions Copyrighted [year]
> + * [name of copyright owner]"
> + *
> + * Contributor(s):
> + *
> + * If you wish your version of this file to be governed by
> only the CDDL or
> + * only the GPL Version 2, indicate your decision by adding
> "[Contributor]
> + * elects to include this software in this distribution under
> the [CDDL or GPL
> + * Version 2] license." If you don't indicate a single
> choice of license, a
> + * recipient has the option to distribute your version of
> this file under
> + * either the CDDL, the GPL Version 2 or to extend the choice
> of license to
> + * its licensees as provided above. However, if you add GPL
> Version 2 code
> + * and therefore, elected the GPL Version 2 license, then the
> option applies
> + * only if the new code is made subject to such option by the
> copyright
> + * holder.
> + */
> +package com.sun.grizzly.util;
> +
> +import java.util.Date;
> +import java.util.logging.Formatter;
> +import java.util.logging.LogRecord;
> +import java.util.logging.Logger;
> +
> +/**
> + * + * Format the record to include the Thread that logged
> the record.
> + * To change the default configuration for java.util.logging
> you will need to
> + * add this in the command line parameters :
> -Djava.util.logging.config.file=myfile
> + * + * Here a sample of what you need to include in myfile
> + * + * #the default logger is this add you can replace it
> with LoggingFormatter
> + * #java.util.logging.ConsoleHandler.formatter =
> java.util.logging.SimpleFormatter
> + * java.util.logging.ConsoleHandler.formatter =
> com.sun.grizzly.util.LoggingFormatter
> + *
> + * @author Sebastien Dionne
> + *
> + */
> +public class LoggingFormatter extends Formatter {
> +
> + private static Logger log =
> Logger.getLogger(LoggingFormatter.class.getName());
> +
> + public LoggingFormatter() {
> + super();
> + }
> +
> + /**
> + * Format the record to include the Thread that logged
> this record.
> + * the format should be
> + * [WorkerThreadImpl-1, Grizzly] 2008-10-08 18:49:59
> [INFO] com.sun.grizzly.Controller:doSelect message
> [WorkerThreadImpl-1, Grizzly] 2008-10-08 18:49:59 [INFO]
> com.sun.grizzly.Controller:doSelect message+ *
> + * @param record The record to be logged into the logger.
> + * + * @return the record formated to be more
> human readable + */
> + public String format(LogRecord record) {
> +
> + // Create a StringBuffer to contain the
> formatted record
> + StringBuffer sb = new StringBuffer();
> +
> +
> sb.append("[").append(Thread.currentThread().getName()).append("]
> ");
> +
> + // Get the date from the LogRecord and add it
> to the buffer
> + Date date = new Date(record.getMillis());
> + sb.append(date.toString()).append(" ");
> +
> + // Get the level name and add it to the buffer
> +
> sb.append("[").append(record.getLevel().getName()).append("] ");
> +
> + // Get Class name
> +
> sb.append(record.getSourceClassName()).append(":");
> +
> + // Get method name
> +
> sb.append(record.getSourceMethodName()).append(" ");
> + + // Get the formatted message
> (includes localization + // and substitution of
> parameters) and add it to the buffer
> + sb.append(formatMessage(record));
> + sb.append("\n");
> +
> + return sb.toString();
> + }
> +
> + /**
> + * Example to test the
> com.sun.grizzly.util.LoggingFormatter
> + * You need to include this parameter in the command line
> + * -Djava.util.logging.config.file=myfile
> + * @param args main parameters
> + */
> + public static void main(String[] args) {
> +
> + log.info <http://log.info>("Info Event");
> +
> + log.severe("Severe Event");
> +
> + // show the thread info in the logger.
> + Thread t = new Thread(new Runnable(){public
> void run(){log.info <http://log.info>("Info Event in
> Thread");}}, "Thread into main");
> + t.start();
> +
> +
> + }
> +}
> \ No newline at end of file
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> commits-unsubscribe_at_grizzly.dev.java.net
> <mailto:commits-unsubscribe_at_grizzly.dev.java.net>
> For additional commands, e-mail:
> commits-help_at_grizzly.dev.java.net
> <mailto:commits-help_at_grizzly.dev.java.net>
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commits-unsubscribe_at_grizzly.dev.java.net
> <mailto:commits-unsubscribe_at_grizzly.dev.java.net>
> For additional commands, e-mail: commits-help_at_grizzly.dev.java.net
> <mailto:commits-help_at_grizzly.dev.java.net>
>
>