WIP: Modified JEventProcessorPODIO::Process to acquire lock only when need… #1531
Conversation
…ed to allow JANA parallelism
for more information, see https://pre-commit.ci
|
|
Please retry analysis of this Pull-Request directly on SonarCloud |
veprbl
requested review from
wdconinc and
nathanwbrei
and removed request for
wdconinc
| lock.lock(); | ||
| if (m_is_first_event) { | ||
| FindCollectionsToWrite(event); | ||
| m_is_first_event = false; | ||
| } | ||
| lock.unlock(); |
There was a problem hiding this comment.
If I understand correctly (m_is_first_event monotonically changing from true to false), the lock can be inside the if, avoiding a lock/unlock after the first event. I'd want to avoid an unnecessary lock since it potentially introduces a wait for the write operation.
The (new) race condition to avoid then is m_is_first_event changing from true to false while the condition is evaluated, so we'd have to do a second locked if inside the unlocked if, but it would virtually never be used.
There was a problem hiding this comment.
You're describing the ABA problem. I think std::call_once is the tool for this purpose. (Just make sure that any code inside call_once is exception free because we've been bitten by bad standard library implementations before)
There was a problem hiding this comment.
any code inside call_once is exception free
The code inside is an entire event processing in EICrecon :=)
There was a problem hiding this comment.
Explanation of double-checked locking and ABA problem for anyone who is interested: https://en.wikipedia.org/wiki/Double-checked_locking#Motivation_and_original_pattern
However, this is an optimization detail that I would avoid for now in favor of the simple, obvious solution. We'd like to verify:
- Consistent results in the output file
- A sensible cores vs throughput curve
- No data races uncovered by valgrind/tsan/whatever
There was a problem hiding this comment.
any code inside call_once is exception free
The code inside is an entire event processing in EICrecon :=)
Nope, just FindCollectionsToWrite() thankfully :)
There was a problem hiding this comment.
Ok, I see. At some point this ran through the whole first event to figure out which output names were activated, right? Or do I remember that wrong?
There was a problem hiding this comment.
It did some wild things in the past for sure. Right now it just returns all available collection names (ignoring whether the corresponding factories have been run or not) and applies inclusions and exclusions.
The goal is to find out. I'm also resuscitating #1069 as part of this effort. |
nathanwbrei
changed the title
Briefly, what does this PR introduce?
This PR introduces changes to the
JEventProcessorPODIO::Processto limit the scope of locks, enabling the use of JANA's parallelism and improving the performance by avoiding unnecessary serialization of events.What kind of change does this PR introduce?
Does this PR introduce breaking changes? What changes might users need to make to their code?
No, this PR does not introduce breaking changes. Users do not need to make any changes to their existing code.
Does this PR change default behavior?
Yes, the PR changes the default behavior by enabling JANA's parallelism through the use of
std::unique_lockwithstd::defer_lock, allowing sections of the code to run in parallel rather than in sequence. The main change is the introduction ofstd::unique_lockwithstd::defer_lockto manage the mutex locking more precisely, allowing for better parallelism by unlocking the mutex during non-critical sections of the code.