On Oct 7, 2004, at 2:51 PM, Christopher Smith wrote:1) You pay the price of constructing std::basic_stream regardless of whether logging is turned on.
Do you have any metrics on the cost of that?
No, but I may have some in a week or so. I'll post them as soon as I do.
I don't think there is a way to avoid that unless you make the log
> stream not derived from the basic_stream and I think that it would > be undesirable to make logstream kind of like, but not exactly, a > basic_stream.
Yeah, it can't be avoided if you make it inherent from basic_stream. Personally, I don't see much of an advantage to having it inherit from basic_stream, and the "is a" relationship seems week to me. Given the disadvantages to this approach, it just doesn't seem worth it.
3) Having parts of the code checking logger->isEnabledFor() and others checking isLoggerEnabled seems aesthetically unclean and seems to me to produce some undesirable behavior. Also, since isLoggerEnabled is not a const (so that it can be changed), you're going to end up with generated code that probably checks for things being enabled several times for each logging event. This means the following code will most likely be more optimal than using logstream:
if (logger->isEnabledFor(myLevel)) { ostringstream buffer; buffer << /* some stuff */; logger->forcedLog(myLevel, buffer.str(), __FILE__, __LINE__); }
Each insertion operator will check a cached boolean value containing the result of IsEnabledFor(), but that should be very cheap. You could probably only update the value at change of level and not a and shave the call to IsEnabledFor. However, if you still called forcedLog() then you could have log events still being fired after a configuration change.
Agreed. However, changes to what levels are enabled are typically concurrent, and as such whether the configuration change happened before or after the event was fired is non-deterministic anyway. I'd argue it's better to be efficient for the 99% case rather than precise about firing events for the 1% case when the configuration is changing. Even in the event of configuration changes, your code only works "correctly" in the event that a log level is disabled while using the stream. If the log level is enabled sometime after the logstream is created but before the logstream is flushed, you're not going to send the event anyway. This kind of inconsistency is my concern about mixing isEnabledFor() and isLoggerEnabled.
It's also worth pointing out that while checking the cached boolean value is going to be quite fast, you are likely going to have multiple branches in the code when only one is necessary.
>4) The whole LOG4CXX_STREAM_DEFINE_INSERTION seems much uglier than using a templated method.
I tried a templated method, but got a lot of ambiguity messages and couldn't come up with anything more elegant at short notice.
I think it is a hard problem to solve if you inherit from basic_stream. To me that's an indication that the inheritence model is bad, but it's quite possible I'm not seeing the forest for the trees here as I have limited insight into log4cxx's complete design, so I'll defer to your judgement on this.
You would need some sort of filter, since you would still like
> insertions that modify the state of the stream, like > "<< scientific ", to still be effective regardless of the current > level.
Why? The idea is to do absolutely nothing if the current level is disabled. Why worry about tracking iostate for a stream that is not used?
--Chris
