logging-log4j2 icon indicating copy to clipboard operation
logging-log4j2 copied to clipboard

ThrowableProxy pre-warms its cache using the detected stack

Open carterkozak opened this issue 5 years ago • 5 comments

This allows us to avoid class loading even when order doesn't match the stack due to exceptions caught and pased from a slightly different codepath. This change also includes a name-based predicate to avoid loading classes types which are known not to provide location information. Unfortunately this mostly invalidates our existing benchmarks which are based on dynamic proxies, but modern java applications use proxies and lambdas heavily enough that I expect a sizable performance improvement.

In a future commit we may consider relying exclusively on classes from the detected stack for location information and entirely avoid class loading.

carterkozak avatar Dec 31 '19 22:12 carterkozak

I need to think about this longer before I'm comfortable with it. Posted for early feedback on the idea.

carterkozak avatar Dec 31 '19 23:12 carterkozak

@carterkozak Have you revisited this? To be honest I looked at the review here but it is hard to tell what effect it might have. It would be great to have some JMH benchmarks to indicate what kind of improvement it provides. Also, I'd like to know if there will be any side-effects or consequences of the change.

rgoers avatar Jul 29 '20 03:07 rgoers

I had forgotten about this change. I'm a little bit afraid of changes to ThrowableProxy because there are a number of cases when multiple classes with the same fully qualified name come from separate loaders in a single trace. I'm not sure this PR would necessarily regress those cases, but I'm sure we could come up with scenarios where each implementation is more accurate. I'll revisit this when I have some more time, it has been a crazy week.

carterkozak avatar Jul 29 '20 04:07 carterkozak

Looking at this again, I think I should extract the predicate for classes which can't be loaded to a separate change. It should be generally helpful because the cost of a failed class load is very high. While I don't think it solves the problem, it may reduce the magnitude. Separately I can document the pros and cons of this approach in which we avoid re-loading classes with names that appear in the collected stack.

carterkozak avatar Jul 30 '20 11:07 carterkozak

Looking at this again, I think I should extract the predicate for classes which can't be loaded to a separate change. It should be generally helpful because the cost of a failed class load is very high. While I don't think it solves the problem, it may reduce the magnitude.

+1; I think that fix would be very helpful.

Apache Spark is upgrading to Log4J 2 in its 3.3 release and during testing I spotted performance problems due to unloadable classes in error stacktraces. Because of this performance issue, I am proposing to change Spark s default Log4J configuration to specify an explicit %ex formatter so we don't use the extended exception information: https://github.com/apache/spark/pull/36747

JoshRosen avatar Jun 02 '22 06:06 JoshRosen