Conversation
|
|
||
| //------------------------------------------------------------------------------ | ||
| bool LumberjackStream::hasPendingMessages() { return m_lj->getMessages().size() > 0; } | ||
| bool LumberjackStream::hasPendingMessages() { |
There was a problem hiding this comment.
This needs to be fixed.
| bool LumberjackStream::isUsingMPI() { return true; } | ||
|
|
||
| MPI_Comm LumberjackStream::comm() { return m_lj->getCommunicator()->comm(); } | ||
| MPI_Comm LumberjackStream::comm() { return m_lj[0]->getCommunicator()->comm(); } |
There was a problem hiding this comment.
This also needs to be fixed.
|
Thanks for clearly laying out the issue, @gberg617 ! I wonder if instead of having multiple LumberjackStream objects each corresponding to different tags that filter out different messages to queue by So instead of these non-combinable messages being added to |
@bmhan12 That's a good point. I think your idea can work. The advantage of your approach is that we don't change the behavior in LumberjackStream, and all the changes stay localized within Lumberjack. However, this would require me to keep the custom Combiner class I wrote in my host code that inspects the tag to determine whether messages should be combined. At first, this seemed somewhat undesirable, but perhaps it's still better than modifying LumberjackStream. |
@bmhan12 After thinking about this, I slightly favor the approach in the current PR. I think it is generally useful to let a single LumberjackStream instance cleanly handle different combiner strategies for one file, instead of creating multiple LumberjackStream objects that all own the same file but use different combiners, or relying on custom Combiner classes like I have been doing. Moving the message sorting responsibility from Lumberjack into LumberjackStream in slic also improves the separation of responsibilities. LumberjackStream is now responsible for formatting and writing sorted messages to a file, and Lumberjack focuses on combining and passing messages between ranks. if you feel this isn't the best approach, I'm happy to discuss it further. |
|
@gberg617 Agreed, separation of responsibilities is a good general practice/design, and having multiple Lumberjacks can helps make things clearer (one Lumberjack for tag A messages, another for tag B messages, etc.). Two things I am concerned about, open to thoughts/discussion: Additional MPI overhead as we increase the number of Lumberjacks. Each Lumberjack object adds additional MPI operations to push messages to root, each Lumberjack object having its own set of messages to manage. I don't know concretely what the overhead is, this might not matter at all. Overhead from processing non-combinable messages separately. With the way We would need to add a blacklist/blocklist like suggested or some other way of indicating the messages for this Lumberjack object in particular do not need to be combined. But based on the point of separation of responsibilities/ Lumberjack's role being to combine messages, if in this scenario we do not want to combine messages, maybe we should not be using Lumberjack/LumberjackStream on these non-combinable messages? SynchronizedStream makes more sense if we need to pass messages between ranks without combining. I'm assuming you know beforehand if a message is combinable or not so you , that assumption might not be realistic. FYI - I added a test branch that implements the blocklist: https://github.com/llnl/axom/tree/feature/han12/lj_blocklist. Feel free to try it out! |
Summary
The goal of this PR is to:
The key ideas are:
This avoids the expensive duplicate checking for tagged messages that are not intended to be combined, and still allows: