[ https://issues.apache.org/jira/browse/LOG4J2-555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13935921#comment-13935921 ]
Bruce Brouwer commented on LOG4J2-555: -------------------------------------- I wanted to give an overview of what my patch is trying to do: # Simplify most logger methods to be one line of code # Allow classes that wrap a logger (e.g. using AbstractLoggerWrapper) to use one line of code to log. To accomplish this, I created all the variants of the log(...) methods that now take a FQCN as the first parameter. # Created an interface called LoggerProvider that AbstractLogger implements. This I think makes it a bit clearer what extra methods are needed beyond what is in the Logger interface to succesfully wrap a logger. This also allows for different (future) logger implementations that don't need to extend AbstractLogger. # I changed LoggerContext to return LoggerProvider. Most people don't tend to go after the LoggerContext, so most people won't see this. But by doing this here, it makes it clear that loggers coming from here are guaranteed to have this extra functionality, and it eliminates some casting done in the code # I tried to fix some of the method parameter ordering to be consistent: fqcn, level, marker, message, ... This patch doesn't cover all cases where this could be done but in the cases were I was changing method signatures anyway, I thought I would make the order consistent. You may notice that this patch includes a change to log4j-jcl Log4jLog.java. This was not absolutely necessary, but it seemed somehow wrong to me to rely on the fact that the method signatures of jcl match up perfectly to log4j2. The patch implementation also prevents leaking a bunch of other public methods into the implementation coming from AbstractLogger and AbstractLoggerWrapper. If others don't agree with me, it would be easy to revert Log4jLog.java back to what it was before, but instead of the first constructor parameter being AbstractLogger, it would change to LoggerProvider. I also just noticed that log4j-to-slf4j SLF4JLogger.logMessage has an unnecessary switch statement. That should probably be cleaned up as switching it to simply call logger.log(FQCN, level, getMarker(marker), ...) would actually be more performant, not only because it eliminates the switch statement, but it would also go through fewer method invocations. Do you want me to provide an updated patch that does this? Finally, I ran PerfTestDriver, just the "Loggers all async" tests. I am not sure what the best thing is to do to communicate performance, but this is what I did. I was kind of surprised to see that this patch is actually consistently more performant than the current log4j2 code (at least that's how I'm reading it). h5. Original: 1. Log4j2: Loggers all async (single thread): throughput: 12,746,429 ops/sec. latency(ns): avg=980.2 99% < 8192.0 99.99% < 1101004.8 (494079 samples) 2. Log4j2: Loggers all async (4 threads): throughput: 6,603,641 ops/sec. latency(ns): avg=3157.4 99% < 5120.0 99.99% < 8703180.8 (3376684 samples) 3. Log4j2: Loggers all async (2 threads): throughput: 3,505,632 ops/sec. latency(ns): avg=1103.5 99% < 5324.8 99.99% < 2516582.4 (2339717 samples) h5. After: 1. Log4j2: Loggers all async (single thread): throughput: 13,612,251 ops/sec. latency(ns): avg=8463.0 99% < 8192.0 99.99% < 2936012.8 (344455 samples) 2. Log4j2: Loggers all async (4 threads): throughput: 7,395,284 ops/sec. latency(ns): avg=2690.5 99% < 5939.2 99.99% < 8231321.6 (5952262 samples) 3. Log4j2: Loggers all async (2 threads): throughput: 4,639,382 ops/sec. latency(ns): avg=1079.2 99% < 6144.0 99.99% < 2464153.6 (2312163 samples) > Location-based functionality broken in AbstractLoggerWrapper subclasses > ----------------------------------------------------------------------- > > Key: LOG4J2-555 > URL: https://issues.apache.org/jira/browse/LOG4J2-555 > Project: Log4j 2 > Issue Type: Bug > Components: API, Core > Affects Versions: 2.0-rc1 > Reporter: Remko Popma > Assignee: Remko Popma > Fix For: 2.0-rc2 > > Attachments: LOG4J2-555-delegate.patch, log4j2-555-bbrouwer-2.patch, > log4j2-555-bbrouwer.patch > > > *How to reproduce* > * Create a custom logger that extends {{AbstractLoggerWrapper}} (or generate > one with the tool attached to LOG4J2-519) > * In the custom logger provide a public method that invokes the {{log(Level, > String)}} method > * Configure a pattern layout that uses location, like %C for the logger FQCN > * From a sample app, call the public method on your custom logger. > * The output will show the class name of the custom logger instead of the > class name of the calling class in the sample application. > *Cause* > {{AbstractLogger}}'s FQCN field is {{static final}} and initialized to > {{AbstractLogger.class.getName()}}. Then, in > {{Log4jLogEvent#calcLocation()}}, when walking over the stack trace elements, > the element _following_ the FQCN is returned. So only loggers that directly > subclass from {{AbstractLogger}} will work correctly. Loggers that inherit > from {{AbstractLoggerWrapper}} are two levels removed from {{AbstractLogger}} > and the {{calcLocation()}} method will not work correctly. > *Solution* > I think {{AbstractLogger}}'s FQCN field should be made non-static, and > initialized to {{getClass().getName()}} in the constructor of > {{AbstractLogger}}. {{Log4jLogEvent#calcLocation()}} can then be modified to > return the {{StackElement}} whose class name matches the FQCN, instead of the > next element. Location-based functionality should then work for arbitrarily > deep subclass hierarchies of AbstractLogger. -- This message was sent by Atlassian JIRA (v6.2#6252) --------------------------------------------------------------------- To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org