[
https://issues.apache.org/jira/browse/LOG4J2-2389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16552354#comment-16552354
]
ASF GitHub Bot commented on LOG4J2-2389:
----------------------------------------
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/195
@rgoers Thanks for the reply and I see the problem. There are 2 problems:
1. If multiple ClassLoader has loaded classes with the same name, they will
interfere each other.
2. We can't clear it from the cache if the class is unloaded.
But I also checked the benchmark. After fixing the bug, the performance has
been improved a lot if there are duplicated classes in the same stack. And it
will improve further more for all kinds of exceptions if the cache is global,
it's very attractive. So I just think if there is a way to solve these problems.
I found the cache map is a map with `String` type key indicating a class.
So why not make the key type as `Class`? The `Class` objects will be different
if they are loaded by different `ClassLoader`s even with the same qualified
name. And as for the `unloading`, we may introduce the `WeakReference` in the
key. When a class is unloaded and collected by GC, the reference will receive a
notice. We can clear the information from the cache map then.
And after all, I still agree only to fix the bug for this time. If there
should be such further optimizations, we can do it later after close study.
Yours.
> fix the CacheEntry map in ThrowableProxy#toExtendedStackTrace to be put and
> gotten with same key
> ------------------------------------------------------------------------------------------------
>
> Key: LOG4J2-2389
> URL: https://issues.apache.org/jira/browse/LOG4J2-2389
> Project: Log4j 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.6.2, 2.7, 2.8, 2.8.1, 2.8.2, 2.9.0, 2.9.1, 2.10.0,
> 2.11.0
> Reporter: LIU WEN
> Priority: Major
>
> * fix the CacheEntry map in ThrowableProxy#toExtendedStackTrace to be put and
> gotten with same key
> * stackTraceElement.toString() returns a string representation of this stack
> trace element, just as MyClass.mash(MyClass.java)
> * stackTraceElement.getClassName() returns the fully qualified name of the
> Class, just as org.apache.logging.log4j.MyClass
> {code:java}
> final String className = stackTraceElement.getClassName();
> ……
> final CacheEntry cacheEntry = map.get(className);
> if (cacheEntry != null) {
> final CacheEntry entry = cacheEntry;
> extClassInfo = entry.element;
> if (entry.loader != null) {
> lastLoader = entry.loader;
> }
> } else {
> final CacheEntry entry = this.toCacheEntry(stackTraceElement,
> this.loadClass(lastLoader, className), false);
> extClassInfo = entry.element;
> map.put(stackTraceElement.toString(), entry);
> if (entry.loader != null) {
> lastLoader = entry.loader;
> }
> }
> {code}
> - The main impact of the problem was that it would increase of frequency of
> loading classes ,which led to the execution of the program be slow down,
> because of the synchronization mechanism in the method loadClass
> - In addition to fixing the problem, I think cache map could be made global,
> instead of a new one for each exception instance.
> {code:java}
> private ThrowableProxy(final Throwable throwable, final Set<Throwable>
> visited) {
> ...
> final Map<String, CacheEntry> map = new HashMap<>();
> this.extendedStackTrace = this.toExtendedStackTrace(stack, map, null,
>
> throwable.getStackTrace());
> ...
> }
> {code}
> - We made the benchmark test to compares the performance of ThrowableProxy
> optimizations for different exception
> - baseline: test `ThrowableProxy` in log4j2, not optimized, with
> bug, for comparison consideration.
> - bugfixed: fixed bug in `ThrowableProxy` accessing the cache.
> - optimized: make the cache global, instead of a new one for each
> exception instance.
> Then we see
> * more than `10` times improvement when the bug is fixed for exceptions with
> stack element class duplicated many times.
> * and about `2` times further improvement when make the cache global
> (compared with log4j2, more than `20` times improvement).
> - benchmark test detail:
> https://github.com/liuwenchn/log4j2-throwableproxy-benchmark
> - PR: https://github.com/apache/logging-log4j2/pull/195
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
