[ 
https://issues.apache.org/jira/browse/LOG4J2-1434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15335479#comment-15335479
 ] 

Luke Butters commented on LOG4J2-1434:
--------------------------------------

That would be better than what we have, although not the most ideal solution.

Thinking about the code written we know that the StringBuilder must never 
escape log4j2, so what we could do is the reverse of how the StringBuilder is 
provided. Using java 8 this becomes:
{code}

class AbstractStringLayout {
 
  protected <T> T doWithStringBuilder(Function<StringBuilder, T> formater) {
     StringBuilder sb = getStringBuilderFromThreadLocal();
     try {
        return formater.apply(sb);
     } finally {
       //No more work will be done with the StringBuilder
       if(sb.capacity() > 1024) {
          removeStringBuilderFromThreadLocal();
       } 
     }
  }

}

{code}

Now our sub class might look like: (from 
https://github.com/apache/logging-log4j2/blob/73b4bcffeccc22fd148b4ccc9c0cba1516dcb519/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/GelfLayout.java#L144
 );
{code}
@Override
    public byte[] toByteArray(final LogEvent event) {
        return doWithStringBuilder((sb) -> {
            final StringBuilder text = toText(event, sb, false);
            final byte[] bytes = getBytes(text.toString());
            return compressionType != CompressionType.OFF && bytes.length > 
compressionThreshold ? compress(bytes) : bytes;
        });
    }
{code}

If we need backward support (ie pre java 8) we could define our own interface 
rather than uses the Function provided by java 8.

On my machine with a relatively simple test of using StringBuilders my proposed 
method results in code that is way faster than re-creating the StringBuilder 
and is close to but is not as fast as the Straight ThreadLocal perhaps because 
if the extra capacity check. Here are my speed results:
{code}
CurrentThreadLocal: 122ms, 100%
new StringBuilder each time: 273ms, 224%
Functional: 153ms, 125%
{code}
In my test I get or create the StringBuilder 1million times.

The Functional way is still not as fast however it avoids memory issues.

> StringBuffer in ThreadLocal can cause excessive memory usage after large log 
> messages
> -------------------------------------------------------------------------------------
>
>                 Key: LOG4J2-1434
>                 URL: https://issues.apache.org/jira/browse/LOG4J2-1434
>             Project: Log4j 2
>          Issue Type: Bug
>    Affects Versions: 2.6.1
>            Reporter: Luke Butters
>            Assignee: Remko Popma
>
> In an effort to speed up logging ThreadLocals have been introduced see 
> LOG4J2-1125 however this does causes memory issues.
> The problem of the ThreadLocal occurs when threads are re-used which is an 
> absolutely valid way of using java. For example an executor service can 
> re-use threads as well as Jetty.
> Below I demonstrate a contrived example of the memory leak:
> {code}
> int stringSize = 1024*1024*10; //~10MB maybe 20MB for UTF-16
>         StringBuilder sb = new StringBuilder(stringSize); 
>         for(int i = 0; i < stringSize; i++) {
>             sb.append('a' + i % 5);
>         }
>         
>         String largeString = sb.toString();
>         
>         sb = null; //Let it be GC'ed
>         ExecutorService es = Executors.newFixedThreadPool(100);
>         final CountDownLatch countDownLatch = new CountDownLatch(100);
>         for(int i = 0; i < 100; i++) {
>             es.execute(()-> {
>                 //Log the big string to demonstrate the issue.
>                 log.fatal(largeString);
>                 
>                 //Ensure we use all 100 of our threads by not releasing this 
> thread yet.
>                 countDownLatch.countDown();
>             }); 
>             
>             //We sleep for 2s so we more easily watch memory growth
>             Thread.sleep(2000);
>         }
> {code}
> I recommend that log4j2 immediately remove the ThreadLocal as a small gain in 
> performance does not outweigh the problems associated with memory leaks. 
> Finally other options for caching the StringBuilder with a ThreadLocal could 
> be considered for example we might re-use StringBuilders that are no larger 
> than 3k while removing the ones which are larger than 3k.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org

Reply via email to