Alex,
I boil the issue down as follows, excessive calls to isXEnable is bad because of the excessive overhead.
Agreed?
There are two solutions to this problem (notice both involve caching the value):
1) cache the value and invalidate based on a notification from the logging framework
2) cache the value and invalidate on an interval bases
Agreed?
Since as far as I recall, the log4j production release does not send notifications we can not do option 1. Option 2 is easy and simple. It took literally 20 minutes to write and test.
Now you seem to be arguing that this code will not be faster, and is a complete waste of effort and will only make geronimo harder to maintain. If this is not your point please clarify. If it is, can you tell me how this code would *ever* be slower unless the interval is too aggressive (currently 3 minutes).
BTW here is my justification. isDebugEnabled looks like this:
public boolean isDebugEnabled() {
if (repository.isDisabled(Level.DEBUG_INT)) {
return false;
}
return Level.DEBUG.isGreaterOrEqual(this.getEffectiveLevel());
}This says that if your log hierarchy has debug enabled at all then call getEffectiveLevel which has the following code:
public Level getEffectiveLevel() {
for (Category c = this; c != null; c = c.parent) {
if (c.level != null) {
return c.level;
}
}
return null; // If reached will cause an NullPointerException.
}If you take a look at level it is defined as follows:
protected volatile Level level;
According to the extensive testing Jeremy and I did on the fastest way to implement the locking system, the most expensive thing in raw java is volatile variables. It was actually faster to synchronize and get the value, but that may reduce concurrency. This was not a problem for locking as it is designed to control concurrency. My code on the other hand, caches the value of this expensive call and keeps it in an non-synchronized non-volatile variable. At noted in the code this is OK because boolean assignment is atomic and at the worst case we will have running threads with an outdated processor cache of the value, which is OK as this is designed to be an out of date cache. So unless you turn off debug for the entire hierarchy there is no way my code can be slower and if you have turned it off for the entire hierarchy, my code should be no slower.
At this point, I am going to stop posting to this thread, because this type of detailed justification for such a small non-critical, zero impact commit eating up all of my development time.
-dain
