You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Jess Holle <je...@ptc.com> on 2008/06/06 02:19:57 UTC
Re: Proposed synchronization changes
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
Re: Proposed synchronization changes
Posted by Jess Holle <je...@ptc.com>.
Jacob Kjome wrote:
> Hi Jess,
>
> I suggest you post a tracking bug report for this so that these changes don't just
> get lost on the list.
I may manage to get to this prior to leaving on vacation. My changes
won't be lost for me at any rate :-)
> Sounds like you did a lot of great work that could be the
> basis for Log4j-2.0. Maybe you can gather up enough steam to make 2.0 a reality.
>
This was a just off and on work over a few days. I'm not sure if I have
the "steam" to make 2.0 a reality. I'd like to see a broader
performance, scalability, and thread safety sweep across log4j, but I'm
not sure that's all of what 2.0 should be nor that I can afford to power
even that much of 2.0 at this time.
> And I agree. JDK1.5+ should be what 2.0 targets. There no compelling reason to
> support previous versions. However, ease of migration would be a big issue. If
> 2.0 changes in ways that make it hard for users migrate from 1.2.x to 2.0, then
> they just won't bother. I'd say as long as the primary APIs stay the same
> (Logger, Level, etc...), there's shouldn't be an issue.
>
Agreed.
A big question as per my other e-mail on this thread is how much
compatibility should be maintained for existing Appenders, Layouts, etc.
I can see that we'd want new improved Appenders and Layouts and base
classes thereof, but could also see it making migration more palatable
if old Appender and/or Layout extensions kept working.
--
Jess Holle
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: Proposed synchronization changes
Posted by Jacob Kjome <ho...@visi.com>.
Hi Jess,
I suggest you post a tracking bug report for this so that these changes don't just
get lost on the list. Sounds like you did a lot of great work that could be the
basis for Log4j-2.0. Maybe you can gather up enough steam to make 2.0 a reality.
And I agree. JDK1.5+ should be what 2.0 targets. There no compelling reason to
support previous versions. However, ease of migration would be a big issue. If
2.0 changes in ways that make it hard for users migrate from 1.2.x to 2.0, then
they just won't bother. I'd say as long as the primary APIs stay the same
(Logger, Level, etc...), there's shouldn't be an issue.
Jake
Jess Holle wrote:
> Jess Holle wrote:
>> 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.
>>
> P.S. My previous AppenderAttachableImpl5 was susceptible to race
> conditions. This one should not be -- at least not any that I decided
> mattered.
>
> --
> Jess Holle
>
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: Proposed synchronization changes
Posted by Jess Holle <je...@ptc.com>.
Jess Holle wrote:
> 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.
>
P.S. My previous AppenderAttachableImpl5 was susceptible to race
conditions. This one should not be -- at least not any that I decided
mattered.
--
Jess Holle
Re: Proposed synchronization changes
Posted by Jess Holle <je...@ptc.com>.
Jess Holle wrote:
>
> 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...
>
I also noticed race issues in the concurrent PatternLayout --
specifically between activateOptions()'s handling of patternConverters
and patternFields and format()'s usage thereof.
Such races are real issues for those of us who allow reconfiguration of
layouts, etc, on-the-fly at any time (via JMX in my case -- my own
MBeans as those in log4j had issues).
--
Jess Holle
Re: Proposed synchronization changes
Posted by Jess Holle <je...@ptc.com>.
Curt Arnold wrote:
> On Jun 5, 2008, at 7:19 PM, Jess Holle wrote:
>> 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.
> Thanks for your contribution.
>
> Would you consider signing a Individual Contributor License Agreement
> (http://www.apache.org/licenses/#clas)? While the Apache license has
> clauses that cover material sent to mailing lists, it is preferred to
> have an ICLA on file, particularly for substantial contributions.
I'll have to look into that -- I've contributed code via various Apache
mailing lists and bug reports and have never been asked this.
> My take is that the contribution breaks compatibility with JDK 1.3 and 1.4
As a whole, yes, it does. The submission could be made Java 1.3/1.4
compatible if there was interest, but I didn't see interest in changes
of this magnitude in 1.2.x.
> and the removal of the synchronization lock on Category could
> potentially cause an existing appender to fail that depended on that
> lock, though I haven't tried to formulate a failure scenario.
I thought on this and could not find a failure scenario, whereas the
Category method in question shows up as a bottleneck without any changes.
> This late in the log4j 1.2.x lifecycle, I'm extremely cautious about
> changes that have potential side effects. So I'd -1 applying these to
> the trunk at least prior to log4j 1.2.16 and after that only on much
> more extensive review that I've been able to give it to this point.
That's fine and good -- which is why I went ahead and used Java 5
features wherever appropriate.
> I would not have a problem creating a branch in the sandbox for these
> modifications. I don't think it is the path to a log4j 2.0, but could
> be convinced otherwise. It is at least some movement which is a good
> thing.
I am trying to understand what 2.0 should be and what the path is. The
concurrent sandbox had some good ideas, but I had issues with the
breadth/number of dependencies there.
One question I have not asked in this vein is whether there is any
desire to maintain an "almost certainly 1.2.x compatible" set of base
classes (e.g. perhaps as a separate log4j12-compat.jar module) in 2.0.
That's essentially what my patch is attempting with AppenderSkeleton and
PatternLayout. Most any substantive change can potentially break
compatibility, but I think these should be rather safe.
By the way, I've not had a chance to benchmark my changes yet...
> @author tags are pretty common in the log4j code base, however they
> are currently discouraged in ASF code as they are seen to contribute
> to a territorialism that some code belongs to that particular
> individual and no one else should touch it. We haven't gone threw and
> removed the existing author tags, but should avoid adding any new ones.
Okay -- my IDE threw those in initially and I didn't know to bother to
remove them.
--
Jess Holle
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: Proposed synchronization changes
Posted by Curt Arnold <ca...@apache.org>.
On Jun 5, 2008, at 7:19 PM, Jess Holle wrote:
> 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.
...
Thanks for your contribution.
Would you consider signing a Individual Contributor License Agreement (http://www.apache.org/licenses/#clas
)? While the Apache license has clauses that cover material sent to
mailing lists, it is preferred to have an ICLA on file, particularly
for substantial contributions.
My take is that the contribution breaks compatibility with JDK 1.3 and
1.4 and the removal of the synchronization lock on Category could
potentially cause an existing appender to fail that depended on that
lock, though I haven't tried to formulate a failure scenario. This
late in the log4j 1.2.x lifecycle, I'm extremely cautious about
changes that have potential side effects. So I'd -1 applying these to
the trunk at least prior to log4j 1.2.16 and after that only on much
more extensive review that I've been able to give it to this point.
I would not have a problem creating a branch in the sandbox for these
modifications. I don't think it is the path to a log4j 2.0, but could
be convinced otherwise. It is at least some movement which is a good
thing.
@author tags are pretty common in the log4j code base, however they
are currently discouraged in ASF code as they are seen to contribute
to a territorialism that some code belongs to that particular
individual and no one else should touch it. We haven't gone threw and
removed the existing author tags, but should avoid adding any new ones.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org