Performance improvements to DefaultThreadContextMap

[jengebr]

This implements a performance optimization described on issue #2319 and detailed discussion of the performance characteristics is present there, including a JMH benchmark w/ results.

Note that building change causes complaints about api versioning, and I doubt I got those flags correct. I was able to build locally by updating the package info and pom to 2.24.0.

[ppkarwasz]

@jengebr,

Yes, bumping the package version to 2.24.0 is the right thing to do. We are using OSGi semantics for package versioning, so every change that adds some public API (like a new class) requires at least a minor version bump.

Unfortunately your UnmodifiableArrayBackedMap can not replace the usage of HashMap in DefaultThreadContextMap, because it will cause memory leaks in servlet environments. The DefaultThreadContextMap uses a thread local field to store the map. If this field contains an instance of a class from a web application classloader (like UnmodifieableArrayBackedMap) instead of a class from the JRE, the entire web application classloader will not be garbage collected upon shutdown.

This potential memory leak problem is the only reason we even keep DefaultThreadContextMap.

The performance improvements in your benchmark are however very promising. Can you check the performance of your DefaultThreadContextMap implementation against CopyOnWriteSortedArrayThreadContextMap? You can force Log4j API to use the latter by adding:

-Dlog4j2.threadContextMap=org.apache.logging.log4j.spi.CopyOnWriteSortedArrayThreadContextMap

to your JVM parameters.

[jengebr]

Ugh, the memory leak problem makes sense, and using a WeakReference to track the instance would potentially lose the state. :( Thank you for explaining.

I'll do the perf comparison you suggest.

[jengebr]

Your point is valid, this is comparable to CopyOnWriteSortedArrayThreadContextMap. Closing.

[jengebr]

@ppkarwasz I had an idea: my map implementation was based on a single object (Object[]), which is a JVM class. If we only put the Object[] into the ThreadLocal, we'd avoid the memory leak and could create custom Maps for not-quite-free, since all they do is wrap the array. Could even turn the copy-and-modify operations into static methods and avoid allocating the map for internal operations.

A similar approach could be applied to the existing SortedArrayStringMap, although the extra internal flags (immutable, iterating, keys, size, threshold, values) would need to combined into a single Array, probably with reserved slots for the flags.

Thoughts?

[ppkarwasz]

@jengebr,

Great idea, it should work. @jvz, do you confirm?

Can you make another implementation of DefaultThreadContextMap (called e.g. WebappThreadContextMap or StringArrayThreadContextMap) and submit it as PR? No need to modify SortedArrayStringMap: this is conceived for systems that have a single classloader or do not reload classloaders (like Spring Boot).

In 3.x we got rid of most ThreadLocals thanks to the Recycler abstraction. For obvious reasons the ThreadContextMap is the only place we use ThreadLocals directly.

Remark: the StringMap-based thread context maps can also be safely used in a Servlet environment. You just need to add Log4j API in the system classloader. Many servlet containers allow such a configuration. For Apache Tomcat I have developed some small extensions in copernik-eu/log4j-plugins to ease such a configuration.

[jvz]

Yeah I think that would work.

[jengebr]

PR #2330 adds StringArrayThreadContextMap wrapping UnmodifiableArrayBackedMap. The new version of UnmodifiableArrayBackedMap differs slightly but the JMH test shows that performance is comparable.