On 18-03-2012 21:50, Matthew Toseland wrote: > On Sunday 18 Mar 2012 23:40:44 Marco Schulze wrote: >> One thing has been bothering me: those 'if (logMINOR) Logger.minor(...', >> and the mess that logging is inside fred. I've written a very simple >> replacement for Logger + associated classes with the following changes: > You're not using the java logging API? First thing I thought, but the Java logging mechanisms are complex (more LoC), at least compared to what I have now. It also lacks those cute error()/debug() methods, so a wrapper would be needed anyway.
> The if(logMINOR) is *ABSOLUTELY VITAL* for performance. You simply cannot get > rid of it. Generating the strings costs significantly more CPU than the rest > of what Freenet does put together. In most cases I've seen so far, either the message is constant, or is interpolated with one or two variables. In those cases where building a message to be logged is actually expensive, directly checking the logger state (Logger.min_severity) saves that contrived prolog used to setup logMINOR and logDEBUG. If string building is really slow (possibly because some object's toString() is going wild), logging methods can be changed to something like: public void Logger.info(Object... objs), and the final string would only be built by the logger if it would actually use it. >> - Log level (renamed to severity) filtering is done by Logging.log(); > Feel free to rename it. I don't see much point though unless you are trying > to use a standard API, which *may* be a good idea. No special reason for the change, I just though Logger.Severity sounded better than Logger.LogLevel. >> - Specific writer classes are replaced by a simple OutputStream, which >> defaults to System.err. Formatting is also unified; > Very bad idea. There are many excellent reasons for the complexity that is > FileLoggerHook. One is that if System.err is blocked (e.g. a full disk), > everything grinds to a halt. Doesn't write() throw an IOException when the disk is full? > Adequate caching is also essential, and I'm not certain whether there are > locking issues; You mean, caching of log messages? Could you elaborate? > and dropping log lines when we can't keep up rather than just slowing down > the rest of the node is a good thing. Sorry, but I heavily disagree with you here. Not only dropping messages makes logs misleading, the output stream should either be unbuffered or flushed after every message. If everything is slowing down because of this, it is either expected (DEBUG or TRACE messages are written), or fred is logging too much above the default INFO threshold. >> - Severity cases are broadened (FATAL, ERROR, WARNING, INFO, DEBUG and >> TRACE), MINOR is mapped to DEBUG, and NORMAL is mapped to INFO; > Using the standard classes is fine by me. The biggest problem is that MINOR and NORMAL aren't being used the same way by all classes, and their use isn't exactly clear. >> - No logging method accepts an Object parameter - hashCode() is not >> exactly useful. > We're not after the hashcode, we're after the class name. We need to be able > to filter by it, and that is essential. The problem is disambiguation, or per-object log threshold? >> Additionally, log rotation will be moved outside (possibly inside Node). > Not a good idea, for reasons of locking, file handle handling, the fact that > you can't rename a file that is open on Windows ... I see. A LogRotator would solve it then. > There are GOOD REASONS for all the complexity, but feel free to tinker with > the API, or give specific arguments other than "this is all so complicated it > *ought to* be possible to do something radically simpler". Not that it *ought to be* simpler, I just think current requirements are a bit exaggerated. >> Currently, the log format is '<severity>\t<message>'. > We need the timestamp, in a great many cases. In my todo list. Which object keeps track of uptime? > > > _______________________________________________ > Devl mailing list > Devl at freenetproject.org > https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://emu.freenetproject.org/pipermail/devl/attachments/20120319/2d9ab660/attachment.html>
