[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13667444#comment-13667444
]
Remko Popma commented on LOG4J2-154:
Just FYI, I'm seeing some nice performance improvements with this change.
1 thread: 33% more throughput (2.4M ops/sec -> 3.2M ops/sec), avg. latency down
630 ns/op -> 450 ns/op
2 threads: 62% more throughput (0.96M ops/sec -> 1.56M ops/sec), avg. latency
down 787 ns/op -> 600 ns/op
(With all loggers async on Solaris 10 (64bit) with JDK1.7.0_06, 4-core Xeon
X5570 dual CPU @2.93Ghz with hyperthreading switched on (16 virtual cores))
Darn! I guess this means I have to redo all the performance testing for the
http://logging.apache.org/log4j/2.x/manual/async.html#Performance page...
That's a lot of work! :-( Oh well... :-)
> 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
>Assignee: 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13667421#comment-13667421
]
Remko Popma commented on LOG4J2-154:
Initial fix for this issue (use copy-on-write map and stack in ThreadContext)
is now committed to trunk (revision 1486482).
This includes the method name changes mentioned in the previous comment above.
Let me know if you disagree with the method name changes.
> 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
>Assignee: 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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() {
> ret
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13667175#comment-13667175
]
Remko Popma commented on LOG4J2-154:
Request for feedback:
In interface org.apache.logging.log4j.spi.ThreadContextMap,
I would like to rename two methods, to clarify their intention and usage:
Current:
{code}
/**
* Get a copy of current thread's context Map.
* @return a copy of the context.
*/
Map getContext();
/**
* Return the actual context Map.
* @return the actual context Map.
*/
Map get();
{code}
Proposed change:
{code}
/**
* Get a non-{@code null} mutable copy of current thread's context Map.
* @return a mutable copy of the context.
*/
Map getCopy();
/**
* Return an immutable view on the context Map or {@code null} if the
context map is empty.
* @return an immutable context Map or {@code null}.
*/
Map getImmutableMapOrNull();
{code}
Thoughts?
> 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
>Assignee: 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(collection);
> }
> public void clear() {
> list.clear();
> }
> public String pop() {
> int index = list.size() - 1;
> if (index >= 0) {
> String re
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13655677#comment-13655677
]
Remko Popma commented on LOG4J2-154:
Ok, thanks for the confirm.
> 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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 asList() {
> return list;
> }
> public void trim(int depth) {
> if (depth < 0) {
> throw new IllegalArgumentException(
> "Maximum stack depth
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13655663#comment-13655663
]
Ralph Goers commented on LOG4J2-154:
Normally CTR is OK. I didn't do that for this because the changes could have a
wide impact and I wanted other eyes to review it. I'm pretty comfortable with
the changes to the ContextMap but the ContextStack concerned me. Personally, I
have no idea why anyone would want to use the ContextStack but since it was
part of Log4j 1 I didn't feel I could drop it.
Feel free to run with this.
> 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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) {
>
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13655505#comment-13655505
]
Remko Popma commented on LOG4J2-154:
Ralph, I would like to take the next step with this issue, but how to proceed
with this? Shall I do what you did before: post a patch and ask for feedback?
Or is commit-then-review ok? Either works for me.
> 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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 asList() {
> return list;
>
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
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());
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());
}
return new MutableThreadContextStack(stack.get());
After:
List list = null;
if (!useStack || (list = stack.get()) == null) {
return new MutableThreadContextStack(new ArrayList());
}
return new MutableThreadContextStack(list);
DefaultThreadContextStack#size()
Before:
return stack.get() == null ? 0 : stack.get().size();
After:
List list = stack.get();
return list == null ? 0 : list.size();
DefaultThreadContextStack#isEmpty()
Before:
return stack.get() == null ? 0 : stack.get().isEmpty();
After:
List 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 list = stack.get();
return list == null ? 0 : list.contains(o);
DefaultThreadContextStack#iterator()
Before:
if (stack.get() == null) {
return new ArrayList().iterator();
} else {
return stack.get().iterator();
}
After:
List 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 list = stack.get();
return stack.get().toArray(new Object[list.size()]);
}
After:
List 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().toArray(ts);
}
return stack.get().toArray(ts);
After:
List 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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(map);
> }
> copy.put(key, value);
> localMap.set(copy);
> }
> Example context stack push: deep copy (expensive but rare)
> public static void push(String message) {
>
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13573977#comment-13573977
]
Remko Popma commented on LOG4J2-154:
Not really, the code in the comment was for beta3, but the attached patches are
for beta4 and should just work with current trunk.
Also, the FastLog4j-v2-for-beta4.zip file attached to LOG4J2-151 contains the
same files. Might save you some work. Sorry if I was unclear.
> 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-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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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 L
[jira] [Commented] (LOG4J2-154) ThreadContext performance improvement: shallow copies for reads, deep copies for writes
[
https://issues.apache.org/jira/browse/LOG4J2-154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13573704#comment-13573704
]
Ralph Goers commented on LOG4J2-154:
The patch you supplied for this in LOG4J2-151 was for beta3. ThreadContext
changed a bit for beta4. I've been working on taking the concepts from your
patch and refactoring the beta4 code to do what you are suggesting.
> 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-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 map = localMap.get();
> Map copy = null;
> if (map == null) {
> copy = new HashMap();
> } else {
> copy = new HashMap(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 getImmutableContext() {
> Map 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 list;
> public ThreadContextStack() {
> list = new ArrayList();
> }
> /**
> * This constructor uses the specified list as its internal data
> * structure unchanged. It does not make a defensive copy.
> */
> public ThreadContextStack(List 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 collection) {
> list = new ArrayList(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 asList() {
> return list;
>
