[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13586642#comment-13586642
]
Remko Popma commented on LOG4J2-154:
------------------------------------
Ralph, sorry for taking so long to get back. Looks good, very clean! Below are
some things I noticed.
ThreadContext: EMPTY_STACK is currently mutable
Before:
public static final ThreadContextStack EMPTY_STACK = new
MutableThreadContextStack(new ArrayList<String>());
After:
public static final ThreadContextStack EMPTY_STACK = new
MutableThreadContextStack(Collections.emptyList());
----
DefaultThreadContextStack:
1. avoid multiple calls to stack.get() within one method as value may have been
changed/removed by another thread between calls.
for example: "if (stack.get() != null) {return stack.get().size();}" may
result in NullPointerExceptions
2. I would use Collections.emptyList and Collections.emptyIterator to avoid
creating ArrayList objects where possible
DefaultThreadContextStack#copy()
Before:
if (!useStack || stack.get() == null) {
return new MutableThreadContextStack(new ArrayList<String>());
}
return new MutableThreadContextStack(stack.get());
After:
List<String> list = null;
if (!useStack || (list = stack.get()) == null) {
return new MutableThreadContextStack(new ArrayList<String>());
}
return new MutableThreadContextStack(list);
DefaultThreadContextStack#size()
Before:
return stack.get() == null ? 0 : stack.get().size();
After:
List<String> list = stack.get();
return list == null ? 0 : list.size();
DefaultThreadContextStack#isEmpty()
Before:
return stack.get() == null ? 0 : stack.get().isEmpty();
After:
List<String> list = stack.get();
return list == null ? 0 : list.isEmpty();
DefaultThreadContextStack#contains(Object o)
Before:
return stack.get() == null ? 0 : stack.get().contains(o);
After:
List<String> list = stack.get();
return list == null ? 0 : list.contains(o);
DefaultThreadContextStack#iterator()
Before:
if (stack.get() == null) {
return new ArrayList<String>().iterator();
} else {
return stack.get().iterator();
}
After:
List<String> list = stack.get();
if (list == null) {
return Collections.emptyIterator(); // avoid creating ArrayList
} else {
return list.iterator();
}
DefaultThreadContextStack#toArray()
Before:
if (stack.get() == null) {
return new String[0];
} else {
List<String> list = stack.get();
return stack.get().toArray(new Object[list.size()]);
}
After:
List<String> list = stack.get();
if (list == null) {
return new String[0];
} else {
return list.toArray(new Object[list.size()]);
}
DefaultThreadContextStack#toArray(T[] ts)
Before:
if (stack.get() == null) {
return new ArrayList<String>().toArray(ts);
}
return stack.get().toArray(ts);
After:
List<String> list = stack.get();
if (list == null) {
return Collections.emptyList().toArray(ts); // avoid creating
new ArrayList
}
return list.toArray(ts);
> ThreadContext performance improvement: shallow copies for reads, deep copies
> for writes
> ---------------------------------------------------------------------------------------
>
> Key: LOG4J2-154
> URL: https://issues.apache.org/jira/browse/LOG4J2-154
> Project: Log4j 2
> Issue Type: Improvement
> Components: Core
> Affects Versions: 2.0-beta3
> Reporter: Remko Popma
> Attachments: LOG4J2-154.patch,
> LOG4J2-154-patch-DefaultThreadContextMap.txt,
> LOG4J2-154-patch-ThreadContextMap.txt, LOG4J2-154-patch-ThreadContext.txt
>
>
> Currently, every time a Log4jLogEvent object is created, a deep copy is made
> of both the context map and the context stack. However, expected usage is
> that only a few objects are pushed onto the stack or put in the context map,
> while the number of LogEvents is in the thousands or millions.
> Essentially, there are far more reads than writes, so using a copy-on-write
> mechanism should give us better performance.
> Example context map put: deep copy (expensive but rare)
> public static void put(String key, String value) {
> if (!useMap) {
> return;
> }
> Map<String, String> map = localMap.get();
> Map<String, String> copy = null;
> if (map == null) {
> copy = new HashMap<String, String>();
> } else {
> copy = new HashMap<String, String>(map);
> }
> copy.put(key, value);
> localMap.set(copy);
> }
> Example context stack push: deep copy (expensive but rare)
> public static void push(String message) {
> if (!useStack) {
> return;
> }
> ContextStack stack = localStack.get();
> ContextStack copy = null;
> if (stack == null) {
> copy = new ThreadContextStack();
> } else {
> copy = stack.copy();
> }
> copy.push(message);
> localStack.set(copy);
> }
> Now, when the Log4jLogEvents are created, they just call
> ThreadContext.getImmutableContext and getImmutableStack. These methods return
> an unmodifiable wrapper around the most recent copy.
> Example for context map:
> public static Map<String, String> getImmutableContext() {
> Map<String, String> map = localMap.get();
> return map == null ? EMPTY_MAP : Collections.unmodifiableMap(map);
> }
> Example for context stack:
> public static ContextStack getImmutableStack() {
> ContextStack stack = localStack.get();
> return stack == null ? EMPTY_STACK : new
> ImmutableStack(stack.asList());
> }
> Note that ThreadContext.ThreadContextStack needs to be changed to contain an
> ArrayList rather than extend it, to facilitate making both deep mutable
> copies and shallow immutable copies.
> private static class ThreadContextStack implements ContextStack {
> private static final long serialVersionUID = 5050501L;
> private List<String> list;
> public ThreadContextStack() {
> list = new ArrayList<String>();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List<String> aList) {
> list = aList; // don't copy!
> }
> /**
> * This constructor copies the elements of the specified collection
> into
> * a new list. Changes to the specified collection will not affect
> this
> * {@code ThreadContextStack}.
> */
> public ThreadContextStack(Collection<String> collection) {
> list = new ArrayList<String>(collection);
> }
> public void clear() {
> list.clear();
> }
> public String pop() {
> int index = list.size() - 1;
> if (index >= 0) {
> String result = list.get(index);
> list.remove(index);
> return result;
> }
> throw new NoSuchElementException("The ThreadContext stack is
> empty");
> }
> public String peek() {
> int index = list.size() - 1;
> if (index >= 0) {
> return list.get(index);
> }
> return null;
> }
> public void push(String message) {
> list.add(message);
> }
> public int getDepth() {
> return list.size();
> }
> public List<String> asList() {
> return list;
> }
> public void trim(int depth) {
> if (depth < 0) {
> throw new IllegalArgumentException(
> "Maximum stack depth cannot be negative");
> }
> while (list.size() > depth) {
> list.remove(list.size() - 1);
> }
> }
> /**
> * Returns a deep copy of this stack.
> */
> public ContextStack copy() {
> return new ThreadContextStack(new ArrayList<>(list));
> }
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
