It seems like it'd be more flexible to tune, too. The current
ThreadLocal approach scales uncontrollably with the number of threads
as it is.
On Mon, 6 Jan 2020 at 12:20, Gary Gregory <garydgreg...@gmail.com> wrote:
>
> 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>
> >
--
Matt Sicker <boa...@gmail.com>
