[GitHub] logging-log4j2 pull request #189: [LOG4J2-2368] Recursive logging doesn't cl...

2018-07-09 Thread remkop 
Github user remkop commented on a diff in the pull request:

https://github.com/apache/logging-log4j2/pull/189#discussion_r201213575
  
--- Diff: 
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
 ---
@@ -114,6 +115,10 @@ public B setHeaderSerializer(final Serializer 
headerSerializer) {
  * @return a {@code StringBuilder}
  */
 protected static StringBuilder getStringBuilder() {
+if (AbstractLogger.getRecursionDepth() > 1) { // LOG4J2-2368
--- End diff --

`AbstractLogger.recursionDepthHolder` is of type `ThreadLocal` 
because that was the simplest data structure that met all the requirements:
* It needs to be thread-local, because when multiple threads are logging 
concurrently each thread has its own recursion depth
* Incrementing and decrementing the counter happens at least once for each 
log message so should be fast and not allocate any objects

An alternative would have been to use `ThreadLocal`. This 
would allow the `AbstractLogger` code to look like 
`getRecursionDepthHolder().incrementAndGet()` instead of 
`getRecursionDepthHolder()[0]++`. 

That's perhaps a bit nicer, but I thought it looked strange to hold a 
thread-safe atomic data structure inside a thread-local. `ThreadLocal` 
felt simpler.


---

[GitHub] logging-log4j2 pull request #189: [LOG4J2-2368] Recursive logging doesn't cl...

2018-07-09 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/logging-log4j2/pull/189


---

[GitHub] logging-log4j2 pull request #189: [LOG4J2-2368] Recursive logging doesn't cl...

2018-07-09 Thread cakofony 
Github user cakofony commented on a diff in the pull request:

https://github.com/apache/logging-log4j2/pull/189#discussion_r201183751
  
--- Diff: 
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
 ---
@@ -114,6 +115,10 @@ public B setHeaderSerializer(final Serializer 
headerSerializer) {
  * @return a {@code StringBuilder}
  */
 protected static StringBuilder getStringBuilder() {
+if (AbstractLogger.getRecursionDepth() > 1) { // LOG4J2-2368
--- End diff --

My guess is Integer would require boxing/unboxing on set/access and 
AtomicInteger incurs a cost for thread safety. The array is a cheap MutableInt. 
@remkop could likely provide a definitive answer.


---

[GitHub] logging-log4j2 pull request #189: [LOG4J2-2368] Recursive logging doesn't cl...

2018-07-09 Thread garydgregory 
Github user garydgregory commented on a diff in the pull request:

https://github.com/apache/logging-log4j2/pull/189#discussion_r201176090
  
--- Diff: 
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
 ---
@@ -114,6 +115,10 @@ public B setHeaderSerializer(final Serializer 
headerSerializer) {
  * @return a {@code StringBuilder}
  */
 protected static StringBuilder getStringBuilder() {
+if (AbstractLogger.getRecursionDepth() > 1) { // LOG4J2-2368
--- End diff --

Curious and somewhat unrelated: Why isn't the recursion depth a Integer or 
AtomicInteger?


---

[GitHub] logging-log4j2 pull request #189: [LOG4J2-2368] Recursive logging doesn't cl...

2018-07-09 Thread cakofony 
GitHub user cakofony opened a pull request:

https://github.com/apache/logging-log4j2/pull/189

[LOG4J2-2368] Recursive logging doesn't clobber cached StringBuidlers



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/cakofony/logging-log4j2 LOG4J2-2368

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/logging-log4j2/pull/189.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #189


commit c213024d33ed2c95ece9fb788ee5fd0e62d0bc5a
Author: Carter Kozak 
Date:   2018-07-09T21:13:20Z

[LOG4J2-2368] Recursive logging doesn't clobber cached StringBuidlers




---