For my own usage I went ahead and completed the a set of changes to
reduce log4j 1.2.x's locking and deadlock potential. The results are
attached and make unabashed use of Java 5 -- as from discussion to this
point it seems clear that there is no interest in making any of these
sorts of changes in log4j 1.2.x yet I felt the need for such
improvements now and only care about Java 5 and later environments.
For anyone interested this patch set makes the following changes:
* Removed the synchronization bottleneck in Category.callAppenders()
and AppenderAttachableImpl.
* Added AppenderAttachableImpl5, a Java 5 specific alternative to
AppenderAttachableImpl, and used CopyOnWriteArrayList to further
reduce locking during append. Used this from Category rather than
AppenderAttachableImpl but left AppenderAttachableImpl around for
any for compatibility with any subclasses as
AppenderAttachableImpl5 does not use a protected Vector field as
subclasses would expect.
* Added PreFormattingLayout interface, which provides methods to
precompute the formatted string, e.g. prior to entering a
synchronization block in existing Appender code, and then use the
precomputed string from within this block.
* Amended AppenderSkeleton to automatically make use of these
methods for any layout implementing PreFormattingLayout.
* Modified PatternLayout to implement PreFormattingLayout.
* Updated DatePatternConverter to be thread-safe yet non-blocking.
* Amended AbsoluteTimeDateFormat's constructors to initialize the
numberFormat field which DateFormat internally assumes is non-null
(e.g. in equals and clone) but does not initialize.
* Addressed thread-safety/race issue in WriterAppender.
* There are probably some other thread-safety/race fixes in there I
forgot about -- I kept noticing these things as I was examining
the code.
Collectively these changes allow:
* Appender implementations involving no locking whatsoever upon
append (or no additional locking beyond the minimum required).
o I created an AppenderSkeleton alternative for our own usage
similar to ConcurrentAppender but different in several respects.
+ ConcurrentAppender relies on all sorts of stuff from
outside the core log4j jar -- I didn't want to.
+ ConcurrentAppender introduces a notion of 'closed'
state and a read-write lock -- I wanted to leave that
to subclasses which need it and not burden others with
this.
+ ConcurrentAppender uses a guard to prevent
re-entrancy. I examined some of my custom appenders
and found this totally unnecessary in some cases --
and thus didn't want this in the base class.
+ Etc...
* Use of existing Appenders without the locking encompassing
PatternLayout formatting.
o This increases multi-threaded throughput and reduces
deadlock potential (which logging from within
message.toString() can all too easily cause today).
o This could be done with other layouts as well, but we are
currently primarily using PatternLayout.
o Again, I chose not to use the PatternLayout from the
concurrent sandbox as it introduces dependencies outside the
core log4j.jar.
As for log4j 2.0, there are a number of important questions to be answered:
* How much of the non-core should be relied upon -- especially for
core capabilities? [I'd the default core appender base class
should be concurrent.] How much should move into the core and
replace existing stuff there?
* Do we really care about anything prior to Java 5 with log4j 2.0?
I'd argue not. log4j 1.2.x works well enough for pre-Java-5 and
Java 5 provides compelling advantages.
* Etc...
--
Jess Holle
# This patch file was generated by NetBeans IDE
# Following Index: paths are relative to: D:\User
Profiles\jessh.PTCNET\Desktop\log4jsvn
# This patch can be applied using context Tools: Patch action on respective
folder.
# It uses platform neutral UTF-8 encoding and \n newlines.
# Above lines and this line are ignored by the patching process.
Index: src/main/java/org/apache/log4j/AppenderSkeleton.java
--- src/main/java/org/apache/log4j/AppenderSkeleton.java Base (BASE)
+++ src/main/java/org/apache/log4j/AppenderSkeleton.java Locally Modified
(Based On LOCAL)
@@ -17,7 +17,6 @@
package org.apache.log4j;
-import org.apache.log4j.Layout;
import org.apache.log4j.spi.Filter;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.OptionHandler;
@@ -40,6 +39,7 @@
/** The layout variable does not need to be set if the appender
implementation has its own layout. */
protected Layout layout;
+ private PreFormattingLayout preFormattingLayout;
/** Appenders are named. */
protected String name;
@@ -226,13 +226,7 @@
* AppenderSkeleton#append} method.
* */
public
- synchronized
void doAppend(LoggingEvent event) {
- if(closed) {
- LogLog.error("Attempted to append to closed appender named ["+name+"].");
- return;
- }
-
if(!isAsSevereAsThreshold(event.getLevel())) {
return;
}
@@ -248,8 +242,26 @@
}
}
+ final PreFormattingLayout preFormattingLayout = this.preFormattingLayout;
+ if ( preFormattingLayout != null )
+ preFormattingLayout.preFormat( event );
+ try
+ {
+ synchronized ( this )
+ {
+ if(closed) {
+ LogLog.error("Attempted to append to closed appender named
["+name+"].");
+ return;
+ }
this.append(event);
}
+ }
+ finally
+ {
+ if ( preFormattingLayout != null )
+ preFormattingLayout.clearPreFormat( event );
+ }
+ }
/**
Set the [EMAIL PROTECTED] ErrorHandler} for this Appender.
@@ -275,6 +287,8 @@
*/
public
void setLayout(Layout layout) {
+ this.preFormattingLayout = ( ( layout instanceof PreFormattingLayout )
+ ? (PreFormattingLayout) layout : null );
this.layout = layout;
}
Index: src/main/java/org/apache/log4j/Category.java
--- src/main/java/org/apache/log4j/Category.java Base (BASE)
+++ src/main/java/org/apache/log4j/Category.java Locally Modified (Based On
LOCAL)
@@ -36,7 +36,7 @@
import org.apache.log4j.spi.LoggerRepository;
import org.apache.log4j.spi.HierarchyEventListener;
import org.apache.log4j.helpers.NullEnumeration;
-import org.apache.log4j.helpers.AppenderAttachableImpl;
+import org.apache.log4j.helpers.AppenderAttachableImpl5;
import java.util.Enumeration;
import java.util.MissingResourceException;
@@ -123,7 +123,7 @@
protected LoggerRepository repository;
- AppenderAttachableImpl aai;
+ AppenderAttachableImpl5 aai;
/** Additivity is set to true by default, that is children inherit
the appenders of their ancestors by default. If this variable is
@@ -159,7 +159,7 @@
public
void addAppender(Appender newAppender) {
if(aai == null) {
- aai = new AppenderAttachableImpl();
+ aai = new AppenderAttachableImpl5();
}
aai.addAppender(newAppender);
repository.fireAddAppenderEvent(this, newAppender);
@@ -200,16 +200,15 @@
int writes = 0;
for(Category c = this; c != null; c=c.parent) {
- // Protected against simultaneous call to addAppender, removeAppender,...
- synchronized(c) {
- if(c.aai != null) {
- writes += c.aai.appendLoopOnAppenders(event);
+ // copy into field, so null check and usage are against same value
+ AppenderAttachableImpl5 cAai = c.aai;
+ if (cAai != null) {
+ writes += cAai.appendLoopOnAppenders(event);
}
- if(!c.additive) {
+ if (!c.additive) {
break;
}
}
- }
if(writes == 0) {
repository.emitNoAppenderWarning(this);
Index: src/main/java/org/apache/log4j/helpers/AbsoluteTimeDateFormat.java
--- src/main/java/org/apache/log4j/helpers/AbsoluteTimeDateFormat.java Base
(BASE)
+++ src/main/java/org/apache/log4j/helpers/AbsoluteTimeDateFormat.java Locally
Modified (Based On LOCAL)
@@ -17,6 +17,7 @@
package org.apache.log4j.helpers;
+import java.text.NumberFormat;
import java.util.Date;
import java.util.Calendar;
import java.util.TimeZone;
@@ -60,11 +61,13 @@
public
AbsoluteTimeDateFormat() {
setCalendar(Calendar.getInstance());
+ setNumberFormat( NumberFormat.getIntegerInstance() ); // DateFormat
assumes this is non-null
}
public
AbsoluteTimeDateFormat(TimeZone timeZone) {
setCalendar(Calendar.getInstance(timeZone));
+ setNumberFormat( NumberFormat.getIntegerInstance() ); // DateFormat
assumes this is non-null
}
private static long previousTime;
Index: src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl.java
--- src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl.java Base
(BASE)
+++ src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl.java Locally
Modified (Based On LOCAL)
@@ -53,21 +53,29 @@
}
/**
- Call the <code>doAppend</code> method on all attached appenders. */
+ Call the <code>doAppend</code> method on all attached appenders.
+ Implementations should be thread safe against other methods on this class.
+ */
public
int appendLoopOnAppenders(LoggingEvent event) {
- int size = 0;
- Appender appender;
-
- if(appenderList != null) {
- size = appenderList.size();
+ // copy into field, so null check and usage are against same value
+ Vector appenderList = this.appenderList;
+ if ( appenderList != null ) {
+ Appender appenders[];
+ synchronized ( appenderList ) {
+ int size = appenderList.size();
+ if ( size == 0 )
+ return 0;
+ appenders = (Appender[]) appenderList.toArray( new Appender[size] );
+ }
+ int size = appenders.length;
for(int i = 0; i < size; i++) {
- appender = (Appender) appenderList.elementAt(i);
- appender.doAppend(event);
+ appenders[i].doAppend(event);
}
- }
return size;
}
+ return 0;
+ }
/**
Index: src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl5.java
--- src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl5.java Locally
New
+++ src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl5.java Locally
New
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.log4j.helpers;
+
+import org.apache.log4j.spi.AppenderAttachable;
+import org.apache.log4j.spi.LoggingEvent;
+import org.apache.log4j.Appender;
+
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+/**
+ A straightforward CopyOnWriteArrayList-based implementation of the
+ [EMAIL PROTECTED] AppenderAttachable} interface.
+ <p>
+ Note that althought the implementation uses CopyOnWriteArrayList (and thus
+ requires Java 5, on the whole it is no more thread safe than
+ AppenderAttachableImpl (which was not thread-safe despite using Vector).
+ The main benefit of using CopyOnWriteArrayList is that
+ appendLoopOnAppenders(LoggingEvent) no longer requires <i>any</i>
+ synchronization or array copying.
+ <p>
+ This class is final simply as I see no need to extend it at this time.
+
+ @author Jess Holle
+ */
+public final class AppenderAttachableImpl5 implements AppenderAttachable {
+
+ /** Array of appenders. */
+ private CopyOnWriteArrayList<Appender> appenderList;
+
+ /**
+ Attach an appender. If the appender is already in the list in
+ won't be added again.
+ */
+ public
+ void addAppender(Appender newAppender) {
+ // Null values for newAppender parameter are strictly forbidden.
+ if(newAppender == null)
+ return;
+
+ CopyOnWriteArrayList<Appender> appenderList;
+ synchronized ( this )
+ {
+ appenderList = this.appenderList;
+ if( appenderList == null )
+ this.appenderList = appenderList = new
CopyOnWriteArrayList<Appender>();
+ }
+ appenderList.addIfAbsent( newAppender );
+ }
+
+ /**
+ Call the <code>doAppend</code> method on all attached appenders.
+ This method is thread-safe against other methods on this class.
+ */
+ public
+ int appendLoopOnAppenders(LoggingEvent event) {
+ // copy into field, so null check and usage are against same value and to
reduce field access
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if ( ( appenderList != null ) && !appenderList.isEmpty() ) {
+ int size = 0;
+ for ( Appender appender : appenderList ) {
+ appender.doAppend(event);
+ ++size;
+ }
+ return size;
+ }
+ return 0;
+ }
+
+ /**
+ Get all attached appenders as an Enumeration. If there are no
+ attached appenders <code>null</code> is returned.
+
+ @return Enumeration An enumeration of attached appenders.
+ */
+ public
+ Enumeration getAllAppenders() {
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if(appenderList == null)
+ return null;
+ else
+ return Collections.enumeration( appenderList );
+ }
+
+ /**
+ Look for an attached appender named as <code>name</code>.
+
+ <p>Return the appender with that name if in the list. Return null
+ otherwise.
+
+ */
+ public
+ Appender getAppender(String name) {
+ if(name == null)
+ return null;
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if(appenderList == null)
+ return null;
+
+ for ( Appender appender : appenderList )
+ if(name.equals(appender.getName()))
+ return appender;
+ return null;
+ }
+
+ /**
+ Returns <code>true</code> if the specified appender is in the
+ list of attached appenders, <code>false</code> otherwise.
+
+ @since 1.2 */
+ public
+ boolean isAttached(Appender appender) {
+ if(appender == null)
+ return false;
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if(appenderList == null)
+ return false;
+
+ for ( Appender a : appenderList )
+ if(a == appender)
+ return true;
+ return false;
+ }
+
+ /**
+ * Remove and close all previously attached appenders.
+ * */
+ public
+ void removeAllAppenders() {
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if(appenderList == null)
+ return;
+ for ( Appender a : appenderList )
+ a.close();
+ appenderList.clear();
+ synchronized ( this )
+ {
+ this.appenderList = null;
+ }
+ }
+
+ /**
+ Remove the appender passed as parameter form the list of attached
+ appenders. */
+ public
+ void removeAppender(Appender appender) {
+ if(appender == null)
+ return;
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if(appenderList == null)
+ return;
+ appenderList.remove(appender);
+ }
+
+ /**
+ Remove the appender with the name passed as parameter form the
+ list of appenders.
+ */
+ public
+ void removeAppender(String name) {
+ if(name == null)
+ return;
+ final CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if(appenderList == null)
+ return;
+ for ( Appender appender : appenderList )
+ if(name.equals(appender.getName())) {
+ appenderList.remove(appender);
+ break;
+ }
+ }
+}
Index: src/main/java/org/apache/log4j/helpers/PatternParser.java
--- src/main/java/org/apache/log4j/helpers/PatternParser.java Base (BASE)
+++ src/main/java/org/apache/log4j/helpers/PatternParser.java Locally Modified
(Based On LOCAL)
@@ -16,9 +16,6 @@
*/
package org.apache.log4j.helpers;
-import org.apache.log4j.helpers.LogLog;
-import org.apache.log4j.helpers.OptionConverter;
-import org.apache.log4j.helpers.AbsoluteTimeDateFormat;
import org.apache.log4j.Layout;
import org.apache.log4j.spi.LoggingEvent;
import org.apache.log4j.spi.LocationInfo;
@@ -426,30 +423,58 @@
}
}
- private static class DatePatternConverter extends PatternConverter {
- private DateFormat df;
- private Date date;
+ private static final class DatePatternConverter
+ extends PatternConverter
+ {
+ private final ThreadLocal<DateFormatAndDate> dateFormatAndDate;
- DatePatternConverter(FormattingInfo formattingInfo, DateFormat df) {
- super(formattingInfo);
- date = new Date();
- this.df = df;
+ DatePatternConverter( final FormattingInfo formattingInfo, final
DateFormat dateFormat )
+ {
+ super( formattingInfo );
+ final Date date = new Date();
+ dateFormatAndDate = new ThreadLocal<DateFormatAndDate>()
+ {
+ @Override
+ final protected DateFormatAndDate initialValue()
+ {
+ return ( new DateFormatAndDate( dateFormat, date ) );
}
+ };
+ }
- public
- String convert(LoggingEvent event) {
- date.setTime(event.timeStamp);
- String converted = null;
- try {
- converted = df.format(date);
+ public String convert( final LoggingEvent event )
+ {
+ return ( dateFormatAndDate.get().convert( event ) );
}
- catch (Exception ex) {
- LogLog.error("Error occured while converting date.", ex);
+
+ // keep separate format and date for each thread to avoid both thread
safety issues and synchronization
+ private static final class DateFormatAndDate
+ {
+ DateFormatAndDate( final DateFormat dateFormat, final Date date )
+ {
+ perThreadFormat = (DateFormat) dateFormat.clone();
+ perThreadDate = (Date) date.clone();
}
- return converted;
+
+ String convert( final LoggingEvent event )
+ {
+ perThreadDate.setTime( event.timeStamp );
+ try
+ {
+ return ( perThreadFormat.format( perThreadDate ) );
}
+ catch ( Exception e )
+ {
+ LogLog.error( "Error occured while converting date.", e );
+ return ( null );
}
+ }
+ private final DateFormat perThreadFormat;
+ private final Date perThreadDate;
+ }
+ }
+
private static class MDCPatternConverter extends PatternConverter {
private String key;
Index: src/main/java/org/apache/log4j/PatternLayout.java
--- src/main/java/org/apache/log4j/PatternLayout.java Base (BASE)
+++ src/main/java/org/apache/log4j/PatternLayout.java Locally Modified (Based
On LOCAL)
@@ -392,7 +392,7 @@
@since 0.8.2 */
-public class PatternLayout extends Layout {
+public class PatternLayout extends Layout implements PreFormattingLayout {
/** Default pattern string for log output. Currently set to the
@@ -409,10 +409,8 @@
protected final int BUF_SIZE = 256;
protected final int MAX_CAPACITY = 1024;
+ private final ThreadLocal<String> preFormatData = new
ThreadLocal<String>(); // assumes no re-entrancy between preFormat and
clearPreFormat, which seems safe
- // output buffer appended to when format() is invoked
- private StringBuffer sbuf = new StringBuffer(BUF_SIZE);
-
private String pattern;
private PatternConverter head;
@@ -440,7 +438,7 @@
controls formatting and consists of a mix of literal content and
conversion specifiers.
*/
- public
+ public synchronized // needs to be synchronized to ensure pattern and head
values correspond to one another
void setConversionPattern(String conversionPattern) {
pattern = conversionPattern;
head = createPatternParser(conversionPattern).parse();
@@ -484,24 +482,36 @@
return new PatternParser(pattern);
}
+ public void preFormat( LoggingEvent loggingEvent )
+ {
+ preFormatData.set( internalFormat( loggingEvent ) );
+ }
/**
Produces a formatted string as specified by the conversion pattern.
*/
public String format(LoggingEvent event) {
- // Reset working stringbuffer
- if(sbuf.capacity() > MAX_CAPACITY) {
- sbuf = new StringBuffer(BUF_SIZE);
- } else {
- sbuf.setLength(0);
+ {
+ final String preFormattedString = preFormatData.get();
+ if ( preFormattedString != null )
+ return ( preFormattedString );
}
+ return ( internalFormat( event ) );
+ }
+ private String internalFormat( LoggingEvent event )
+ {
+ final StringBuffer sbuf = new StringBuffer( MAX_CAPACITY );
PatternConverter c = head;
-
while(c != null) {
c.format(sbuf, event);
c = c.next;
}
return sbuf.toString();
}
+
+ public void clearPreFormat( LoggingEvent loggingEvent )
+ {
+ preFormatData.remove();
}
+}
Index: src/main/java/org/apache/log4j/PreFormattingLayout.java
--- src/main/java/org/apache/log4j/PreFormattingLayout.java Locally New
+++ src/main/java/org/apache/log4j/PreFormattingLayout.java Locally New
@@ -0,0 +1,24 @@
+package org.apache.log4j;
+
+
+import org.apache.log4j.spi.LoggingEvent;
+
+
+/**
+ * Interface which when implemented by a Layout implies that the formatting can
+ * be precomputed and retained.
+ *
+ * @author Jess Holle
+ */
+public interface PreFormattingLayout
+{
+ /** To be called prior to entering any synchronization blocks but after one
is
+ * sure 'loggingEvent' is to be logged.
+ */
+ public void preFormat( LoggingEvent loggingEvent );
+
+ /** Should always be called after leaving all synchronization if and only if
+ * preFormat() has been called.
+ */
+ public void clearPreFormat( LoggingEvent loggingEvent );
+}
Index: src/main/java/org/apache/log4j/WriterAppender.java
--- src/main/java/org/apache/log4j/WriterAppender.java Base (BASE)
+++ src/main/java/org/apache/log4j/WriterAppender.java Locally Modified (Based
On LOCAL)
@@ -299,7 +299,8 @@
@since 0.9.0 */
protected
void subAppend(LoggingEvent event) {
- this.qw.write(this.layout.format(event));
+ final Layout layout = this.layout; // copy to local variable so layout
does not change herein
+ this.qw.write(layout.format(event));
if(layout.ignoresThrowable()) {
String[] s = event.getThrowableStrRep();
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
