-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
If SecondaryEventProvider has no modules do not create a task #33125
Conversation
edm::syncWait is always required to create at least one TBB task and then wait on it (which can cause other modules to be run on top of the module using SecondaryEventProvider). To avoid that in the case where there are no modules in SecondaryEventProvider we first check that condition and if so immediately return from the member functions.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33125/21477
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: Mixing/Base @cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28f7b2/13387/summary.html Comparison SummarySummary:
|
@cms-sw/simulation-l2 this is critical to fixing problems in the IB. Please review. |
@Dr15Jones ,should we try enabling |
Sure |
enable threading |
please test workflow 250406.17 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-28f7b2/13411/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
@smuzaffar Looking at the testing output I see that '250406.17' was only run for the single threaded case, not the multi-threaded case. |
So workflow 250202.181 is crashing in the IB but ran fine under the THREADING test for this pull request: |
@civanch, @mdhildreth can you please review this? This is critical to fix failures in IBs |
+1 |
you are right @Dr15Jones , threading job was only running |
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
@smuzaffar rerunning the test with that workflow is nice but not actually necessary. One of the workflows run in threading mode also exhibits the problem in the IB but worked fine here. |
@silviodonato , @qliphy , this is fully signed. Can we get this in IB ? |
+1 |
PR description:
edm::syncWait is always required to create at least one TBB task and then wait on it (which can cause other modules to be run on top of the module using SecondaryEventProvider). To avoid that in the case where there are no modules in SecondaryEventProvider we first check that condition and if so immediately return from the member functions.
This fixes the problem where the Timing service and the MessageLogger were both failing.
PR validation:
The code compiles and running step 1 of workflow 250406.17 using 4 threads, which was failing each time it was run, succeded 3 times in a row when run using the fix.