I like the idea of centrally controlling these objects. This should make
resource monitoring easier as well.
Gary
On Mon, Jan 6, 2020, 13:09 Matt Sicker <boa...@gmail.com> wrote:
> Would it be useful to implement some sort of buffer pool for
> StringBuilders and ByteBuffers? Could likely copy code from netty's
> util library (ByteBuf et al.) or reuse stuff from commons-pool if
> needed. This would work properly in applications, servlets, and even
> reactive streams and lightweight threads later on.
>
> On Tue, 31 Dec 2019 at 03:22, Volkan Yazıcı <volkan.yaz...@gmail.com>
> wrote:
> >
> > On Mon, Dec 30, 2019 at 10:15 PM Carter Kozak <cko...@ckozak.net> wrote:
> > > Beyond StringBuilder instances, we attempt to clear references
> > > from all thread local references to avoid substantial overhead. In
> > > practice this works nicely because it reinforces java performance
> > > characteristics. Java threads are fairly memory expensive (not to
> > > mention the cost of initialization) so the threadlocal object overhead
> > > from log4j tends to be inconsequential by comparison. Applications
> > > in memory constrained environments already have relatively few
> > > threads, and applications which constantly create and destroy threads
> > > tend not to worry about the performance of creating log events
> > > because it's inexpensive compared to thread initialization.
> > >
> > > Have you observed a problem? We've found and resolved a few issues
> > > over the last year or so where references were held longer than
> > > expected. If you're aware of places we're using more memory than we
> > > should, please file a ticket :-)
> >
> > AFAIC, the only TLA in Log4j 2.0 core violating
> > log4j2.enableThreadlocals flag is
> > AbstractStringLayout#getStringBuilder(). Given AbstractStringLayout is
> > used by many internal (HTML, XML, JSON, YAML, Pattern, Gelf, Syslog)
> > and external (ECS) layouts, the fix will incur a significant
> > performance penalty. I wouldn't be surprised if we start receiving
> > performance regression bug reports from users after releasing such a
> > fix, since a notable amount of Log4j 2.0 users, to the best of my
> > knowledge, are using it in JEE context (e.g., Spring WebMvc with
> > Tomcat backend) where ENABLE_THREADLOCALS are disabled due to the
> > present IS_WEB_APP condition:
> >
> > o.a.l.l.u.Constants.ENABLE_THREADLOCALS =
> > !IS_WEB_APP &&
> > PropertiesUtil
> > .getProperties()
> > .getBooleanProperty("log4j2.enable.threadlocals", true);
> >
> > Created LOG4J2-2753[1] for this issue.
> >
> > The reason I started the discussion is, in log4j2-logstash-layout, I
> > am aiming for the fastest approach, always. The performance comparison
> > is JMH-driven, where all competitors (LogstashLayout, EcsLayout,
> > JsonLayout, etc.) are fine tuned for fairness. There I try to play
> > fair, but neither Log4j 2.0 JsonLayout, nor EcsLayout of Elastic does
> > that; they perform TLA without taking log4j2.enableThreadlocals flag
> > into account. This leads me to questionthe presence of such a flag in
> > the first place. Why don't we just remove the
> > log4j2.enableThreadlocals flag? What are its use cases?
> >
> > [1] https://issues.apache.org/jira/browse/LOG4J2-2753
>
>
>
> --
> Matt Sicker <boa...@gmail.com>
>
