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 ca...@apache.org on 2006/09/01 06:50:45 UTC
svn commit: r439177 - in /logging/log4j/trunk/src/java/org/apache/log4j:
scheduler/Scheduler.java spi/LocationInfo.java
Author: carnold
Date: Thu Aug 31 21:50:45 2006
New Revision: 439177
URL: http://svn.apache.org/viewvc?rev=439177&view=rev
Log:
Bug 34762: Scheduler uses notify not notifyAll
Modified:
logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java
logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java
Modified: logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java
URL: http://svn.apache.org/viewvc/logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java?rev=439177&r1=439176&r2=439177&view=diff
==============================================================================
--- logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java (original)
+++ logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java Thu Aug 31 21:50:45 2006
@@ -82,7 +82,7 @@
// if the job is the first on the list, then notify the scheduler thread
// to schedule a new job
if(i == 0) {
- this.notify();
+ this.notifyAll();
}
return true;
} else {
@@ -154,11 +154,11 @@
jobList.add(i, newSJE);
// if the jobList was empty, then notify the scheduler thread
if(i == 0) {
- this.notify();
+ this.notifyAll();
}
}
- public void shutdown() {
+ public synchronized void shutdown() {
shutdown = true;
}
Modified: logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java
URL: http://svn.apache.org/viewvc/logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java?rev=439177&r1=439176&r2=439177&view=diff
==============================================================================
--- logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java (original)
+++ logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java Thu Aug 31 21:50:45 2006
@@ -42,7 +42,7 @@
* without real location info available.
* @since 1.3
*/
- public static LocationInfo NA_LOCATION_INFO = new LocationInfo(NA, NA, NA, NA);
+ public static final LocationInfo NA_LOCATION_INFO = new LocationInfo(NA, NA, NA, NA);
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: svn commit: r439177 - in /logging/log4j/trunk/src/java/org/apache/log4j: scheduler/Scheduler.java spi/LocationInfo.java
Posted by Curt Arnold <ca...@apache.org>.
On Sep 1, 2006, at 9:50 AM, Claudio Corsi wrote:
> Curt,
>
> I've read that using notifyAll compared to notify causes a
> performance degradation. Have you run any performance tests
> comparing this change?
>
> --Claudio
I did not do any performance tests on the change. The change is in
the Scheduler class which was developed for use by the configuration
file monitors that would reconfigure log4j when a change in the
configuration file is detected. In this case, correctness is much
more important than performance. The scheduler has been observed to
fail tests on Gump and the use of notify() when notifyAll() is needed
might have been responsible. I was hoping that the change would
clean up the Gump failures but it appears that it has not.
From section 3.2.4 (pg 192) of "Concurrent Programming in Java,
Second Edition":
>
> In open, extensible designs, the conditions under which notify
> apply are rather special and fragile. The use of notify, and
> optimizations of guarded conditions more generally, are common
> sources of error. As a general design tactic, it is a better idea
> to isolate uses of notify to concurrency control utility classes
> that can be heavily optimized and carefully reviewed and tested.
Getting something like org.apache.log4j.Scheduler done right is
hard. If we were targeting JDK 1.5, it would be a obvious to drop
that class and use java.util.concurrent.ScheduledExecutorServer. If
we were to press on with log4j 1.3 and in light of the on-going test
failures on Gump, we would need to consider either rolling back
FileWatchdog to the log4j 1.2 implementation or take a hard look at
org.apache.log4j.Scheduler.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: svn commit: r439177 - in /logging/log4j/trunk/src/java/org/apache/log4j:
scheduler/Scheduler.java spi/LocationInfo.java
Posted by Claudio Corsi <cc...@progress.com>.
Curt,
I've read that using notifyAll compared to notify causes a performance
degradation. Have you run any performance tests comparing this change?
--Claudio
carnold@apache.org wrote:
> Author: carnold
> Date: Thu Aug 31 21:50:45 2006
> New Revision: 439177
>
> URL: http://svn.apache.org/viewvc?rev=439177&view=rev
> Log:
> Bug 34762: Scheduler uses notify not notifyAll
>
> Modified:
> logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java
> logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java
>
> Modified: logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java
> URL: http://svn.apache.org/viewvc/logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java?rev=439177&r1=439176&r2=439177&view=diff
> ==============================================================================
> --- logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java (original)
> +++ logging/log4j/trunk/src/java/org/apache/log4j/scheduler/Scheduler.java Thu Aug 31 21:50:45 2006
> @@ -82,7 +82,7 @@
> // if the job is the first on the list, then notify the scheduler thread
> // to schedule a new job
> if(i == 0) {
> - this.notify();
> + this.notifyAll();
> }
> return true;
> } else {
> @@ -154,11 +154,11 @@
> jobList.add(i, newSJE);
> // if the jobList was empty, then notify the scheduler thread
> if(i == 0) {
> - this.notify();
> + this.notifyAll();
> }
> }
>
> - public void shutdown() {
> + public synchronized void shutdown() {
> shutdown = true;
> }
>
>
> Modified: logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java
> URL: http://svn.apache.org/viewvc/logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java?rev=439177&r1=439176&r2=439177&view=diff
> ==============================================================================
> --- logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java (original)
> +++ logging/log4j/trunk/src/java/org/apache/log4j/spi/LocationInfo.java Thu Aug 31 21:50:45 2006
> @@ -42,7 +42,7 @@
> * without real location info available.
> * @since 1.3
> */
> - public static LocationInfo NA_LOCATION_INFO = new LocationInfo(NA, NA, NA, NA);
> + public static final LocationInfo NA_LOCATION_INFO = new LocationInfo(NA, NA, NA, NA);
>
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org