-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improves support of log4j2.enableThreadlocals
property
#2550
Conversation
If the `log4j2.enableThreadlocals` property is set to `false` Log4j should clear all `ThreadLocal`s from the calling thread before execution finishes. A similar rule is also applied to `ThreadContext`: assuming the user takes care of leaving an empty map and stack at the end of the execution, all the `ThreadLocal`s should be cleared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing ENABLE_THREADLOCALS
checks
In several places, I see TL#remove()
with only a ifEmpty()
check. That is, ENABLE_THREADLOCALS
is not taken into account. Why?
Performance-related changes without any empirical evidence
The PR description states the following:
While the difference between
remove()
andset(null)
is almost imperceptible in a classical threading model, when virtual threads are used the difference MIGHT BE considerable.
This PR removes 94 and adds 425 LoC. Have we evaluated the assumption using benchmarks? It might be true that we will be shaving off some bytes from the thread stack, but aren't we also introducing an extra overhead since TLs need to be allocated again and again each time?
@@ -45,7 +46,7 @@ | |||
public final class StructuredDataFilter extends MapFilter { | |||
|
|||
private static final int MAX_BUFFER_SIZE = 2048; | |||
private static ThreadLocal<StringBuilder> threadLocalStringBuilder = new ThreadLocal<>(); | |||
private static final ThreadLocal<StringBuilder> threadLocalStringBuilder = new ThreadLocal<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be null
if !Constants.ENABLE_THREADLOCALS
?
I ran some benchmarks a while ago. While I don't remember the numbers, the overhead due to TL deletion and creation was statistically significant. The removal of TLs for the |
If the
log4j2.enableThreadlocals
property is set tofalse
Log4j should clear allThreadLocal
s from the calling thread before execution finishes.The same rule should apply to user methods that correctly use the
ThreadLocal
interface. Code like this:should perform a call to
ThreadLocal#remove
before exiting.While the difference between
threadLocal.remove()
andthreadLocal.set(null)
is almost imperceptible in a classical threading model, when virtual threads are used the difference might be considerable.This PR:
ThreadContextMap
andThreadContextStack
implementations (exceptGarbageFreeSortedArrayThreadContextMap
) so that the context map/stack is cleared if the map/stack is empty.ThreadLocal
s used to detect recursion to callThreadLocal#remove
when the recursion counter reaches 0.ThreadLocal
used for object pooling to respect thelog4j2.enableThreadlocals
property.Remark: This PR does not address LOG4J-2753 that will be fixed in another PR.
Part of #2525.