google
Log4j2LoggerBackend performance improvements
There are some inefficiencies in the Log4j2LoggerBackend, and I have a couple of PRs to make it faster.
I saw drastic improvements to throughput (7 times faster in my microbenchmark), and large reductions in allocations, when running performance tests against the Log4j2LoggerBackend after applying these patches:
- https://github.com/google/flogger/pull/418
- https://github.com/google/flogger/pull/419/
I have structured this as two PRs - the first one I hope will be uncontroversial. The second one may need some discussion.
I'm not sure that any of us know anything about log4j :( The PRs both look likely to be reasonable, but it's hard for us to be confident without further knowledge, and it's hard to set aside the time to accumulate that knowledge when we're not log4j users. I wish I had something more encouraging to say.
Thanks @cpovirk - I understand.
The performance improvements are pretty significant (7 times improved throughput, 7 times reduced latencies, and 6 times reduced allocations) in my microbenchmark. Would the changes be useful to the teams (or team) in google who use the log4j2 backend? Sorry if this sounds pushy...
Otherwise, if you can think of anything else that I could do that could help you with this, please let me know, but I totally understand the circumstances.
Sorry, I should have at least replied here last month.
A single affected team, sadly, means not only that the potential direct benefit to Google is small but also that it's harder for us to be confident that the change works in all cases. (I do expect that it would; I'm just more conservative when considering a change that involves a library that I know nothing about (log4j2) and that only some of us are especially familiar with the internals of (Flogger itself).) I was reminded just now, for example, that ThreadLocal (from #419) is complicated and surrounded by misinformation.
It's still possible that we'd come back for this someday, but I'm not sure what would have to change to make that happen :(
Thanks @cpovirk - understood. And, yep, I totally get that the ThreadLocal changes in https://github.com/google/flogger/pull/419 maybe controversial - that is why its in a separate PR.
Thanks for taking the time to respond. I'll keep an eye on this in case anything changes!