-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
LOG4J2-3068 - JCToolsBlockingQueueFactory can use MpscBlockingConsumerArrayQueue #485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Bumping JCTools version is necessary due to some recent fixes around one of the used qs. It's a shame we're exposing a blocking q API there, or we could've used JCTools/JCTools#340 to batch consume from the queue. |
jvz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a manual entry for this as well? With the other options for this plugin in https://github.com/apache/logging-log4j2/blob/master/src/site/asciidoc/manual/appenders.adoc somewhere
|
@franz1981, thanks so much for the contribution. Would you mind updating |
|
Thanks for the contribution @franz1981, your PRs are appreciated as always! I normally handle the |
We could support a lighter weight API with the ability to utilize both BlockingQueue (retaining backwards compatibility) as well as JCTools/JCTools#340, what do you think? Alternatively a separate JCToolsAsyncAppender is an option. |
Done!
I would go into this direction instead :)
Thinking about a If the unbounded q path is feasable, on |
|
Our fully async disruptor implementation is indeed very difficult to beat in benchmarks, largely for the reasons you've mentioned, also due to the tighter control of the threading model we get by integrating it directly into the logger system (See AsyncLoggerContext, AsyncLogger) rather than applying it to individual appenders (which requires defensive copies of LogEvent objects). This makes it more difficult to like-for-like compare a jctools implementation to the disruptor implementation.
Mandatory? That's difficult to say. In the general case I'd say so, however many services push events into complex logging pipelines which handle event ordering based on timestamps or some other identifier, so it doesn't matter so much what order events are produced in (depending on the system, of course!)
Agreed, this is a big one for us.
The default configuration doesn't take advantage of this property, however configuring most appenders This reminds me, I had meant to research the feasibility of something like libaio for logging file i/o, I recall that you may have some experience with the java bindings? ;-) I imagine eventually io_uring may allow us to batch appends to multiple files into a single syscall, this could be huge as it's common to have some subset of events recorded to multiple files, but I admittedly haven't read as much about how this works in practice as I should.
Thar be dragons! It would be remarkably easy to OOM with an unbounded queue if used incorrectly -- in production systems it's often safer to increase the ringbuffer size and use a discarding policy when the queue is filled, but I don't mind allowing the option for an unbounded queue (on AsyncAppender one can already configure a LinkedBlockingQueue with Integer.MAX_VALUE capacity!). I do like the idea of a queue with a dynamic footprint, at work I use a large constant size ringbuffer which accounts for a large chunk of system memory utilization regardless of load or expectations for the service. |
libaio need some JNI, as io_uring, but IMO io_uring is to be preferred because more modern. If it worths I've already implemented a version of libaio that minimize the usage of JNI (in favour of Unsafe now and VarHandle/Foreign Access in the future) ie apache/artemis-native#10 |
|
@carterkozak do we have some benchmark to run this PR vs the baseline and see if it worths? |
|
Of course, I’d be happy to run the benchmarks! Unfortunately I’m not sure I’ll be able to get to it this week with other things going on. |
|
Sorry for the delay, @franz1981! This PR rebased to master
With more threads: Master without this PR
With more threads: Benchmark ImplementationDraft PR with my benchmark changes used for comparison here: #556 |
|
This was closed automatically by Github because we renamed the |
https://issues.apache.org/jira/browse/LOG4J2-3068