Use tbb::task_group::defer in FinalWaitingTask #38715

Dr15Jones · 2022-07-12T21:47:31Z

PR description:

This will allow the task_group wait to stay open until the FinalWaitingTask is recycled
Added FinalWaitingTask::wait method to handle the usual usage pattern of throwing after wait if needed.

PR validation:

Framework unit tests pass.

cmsbuild · 2022-07-12T21:56:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38715/31012

This PR adds an extra 88KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Concurrency/interface/WaitingTask.h modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Concurrency/test/test_catch2_WaitingTaskChain.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Concurrency/test/waitingtasklist_t.cppunit.cpp modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/interface/maker/Worker.h modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/src/EventProcessor.cc modified in PR(s): Concurrently construct Source while making Modules #38603, Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/src/EventSetupsController.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/src/ProductResolvers.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add Transformer ability to modules #38454
- File FWCore/Framework/src/Worker.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add Transformer ability to modules #38454
- File FWCore/Framework/test/dependentrecord_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/test/esproducer_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/test/eventsetup_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/test/eventsetuprecord_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/test/fullchain_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621
- File FWCore/Framework/test/global_filter_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/global_outputmodule_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/global_producer_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/limited_filter_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/limited_outputmodule_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/limited_producer_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/one_outputmodule_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/stream_filter_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/stream_producer_t.cppunit.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621, Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/TestProcessor/src/TestProcessor.cc modified in PR(s): Have WaitingTask hold an exception_ptr directly #38621

cmsbuild · 2022-07-12T21:56:33Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

FWCore/Concurrency (core)
FWCore/Framework (core)
FWCore/TestProcessor (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@makortel, @wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2022-07-13T12:47:42Z

Please test

cmsbuild · 2022-07-13T19:47:12Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d96a8e/26204/summary.html
COMMIT: 400301f
CMSSW: CMSSW_12_5_X_2022-07-13-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38715/26204/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3653966
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3653930
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 49 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2022-07-13T20:31:58Z

Could you rebase so the first commit (which was merged for the previous IB) would get dropped?

- This will allow the task_group wait to stay open until the FinalWaitingTask is recycled - Added FinalWaitingTask::wait method to handle the usual usage pattern of throwing after wait if needed.

Dr15Jones · 2022-07-13T20:54:22Z

please test

Dr15Jones · 2022-07-13T20:58:40Z

@makortel rebased

cmsbuild · 2022-07-13T21:01:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38715/31033

This PR adds an extra 8KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/src/EventProcessor.cc modified in PR(s): Concurrently construct Source while making Modules #38603
- File FWCore/Framework/test/global_filter_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/global_outputmodule_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/global_producer_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/limited_filter_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/limited_outputmodule_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/limited_producer_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708, Add Transformer ability to modules #38454
- File FWCore/Framework/test/one_outputmodule_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/stream_filter_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708
- File FWCore/Framework/test/stream_producer_t.cppunit.cc modified in PR(s): Add prefetching ActivityRegistry signals for more transitions #38708

cmsbuild · 2022-07-13T21:01:26Z

Pull request #38715 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again.

makortel · 2022-07-13T21:26:07Z

FWCore/Concurrency/interface/FinalWaitingTask.h

+  public:
+    FinalWaitingTask() = delete;
+    explicit FinalWaitingTask(tbb::task_group& iGroup)
+        : m_group{&iGroup}, m_handle{iGroup.defer([]() {})}, m_done{false} {}


I wonder if always adding an empty deferred task could add any visible overhead?

It is only used when shutting down the event loop so I doubt it can be seen at all.

makortel · 2022-07-13T21:27:53Z

FWCore/Concurrency/interface/FinalWaitingTask.h

+
+    [[nodiscard]] bool done() const noexcept { return m_done.load(); }
+
+    void wait() {


Do we care about having similar nomenclature with SYCL queues? (asking since SYCL/DPC++ is part of the oneAPI) There the wait() does not throw, and one has to ask wait_and_throw() explicitly to have exceptions (from asynchronous processing) to throw.

I don't think we care at all. I went with consistency with tbb::task_group::wait which can throw.

Right, consistency with oneTBB's nomenclature is more important.

cmsbuild · 2022-07-14T06:16:36Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d96a8e/26217/summary.html
COMMIT: 5de7db4
CMSSW: CMSSW_12_5_X_2022-07-13-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38715/26217/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

@arsahasransu Add EGamma calo ID variable cut to Run 3 Scouting Electrons #38602
@bsunanda Run3-gex136E Try too remove compilation warnings in PhysicsTools/PatAlgos #38670

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d96a8e/26217/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d96a8e/26217/git-merge-result

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3653966
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3653930
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 49 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2022-07-14T13:46:51Z

+1

cmsbuild · 2022-07-14T13:47:10Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-07-15T08:44:04Z

+1

cmsbuild added this to the CMSSW_12_5_X milestone Jul 12, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Jul 12, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 12, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 13, 2022

cmsbuild mentioned this pull request Jul 13, 2022

Add prefetching ActivityRegistry signals for more transitions #38708

Merged

cmsbuild added tests-approved and removed tests-started labels Jul 13, 2022

Use tbb::task_group::defer in FinalWaitingTask

5de7db4

- This will allow the task_group wait to stay open until the FinalWaitingTask is recycled - Added FinalWaitingTask::wait method to handle the usual usage pattern of throwing after wait if needed.

Dr15Jones force-pushed the useTaskGroupDefer branch from 400301f to 5de7db4 Compare July 13, 2022 20:53

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Jul 13, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 13, 2022

cmsbuild removed the code-checks-pending label Jul 13, 2022

cmsbuild added the code-checks-approved label Jul 13, 2022

makortel reviewed Jul 13, 2022

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jul 14, 2022

makortel mentioned this pull request Jul 14, 2022

Use tbb::task_group::defer in FinalWaitingTask cms-sw/framework-team#414

Closed

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Jul 14, 2022

cmsbuild added orp-approved and removed orp-pending labels Jul 15, 2022

cmsbuild merged commit 4437e0f into cms-sw:master Jul 15, 2022

Dr15Jones deleted the useTaskGroupDefer branch July 18, 2022 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use tbb::task_group::defer in FinalWaitingTask #38715

Use tbb::task_group::defer in FinalWaitingTask #38715

Dr15Jones commented Jul 12, 2022

cmsbuild commented Jul 12, 2022

cmsbuild commented Jul 12, 2022

Dr15Jones commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

makortel commented Jul 13, 2022

Dr15Jones commented Jul 13, 2022

Dr15Jones commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

makortel Jul 13, 2022

Dr15Jones Jul 13, 2022

makortel Jul 13, 2022

Dr15Jones Jul 13, 2022

makortel Jul 13, 2022

cmsbuild commented Jul 14, 2022

makortel commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

perrotta commented Jul 15, 2022


		[[nodiscard]] bool done() const noexcept { return m_done.load(); }

		void wait() {

Use tbb::task_group::defer in FinalWaitingTask #38715

Use tbb::task_group::defer in FinalWaitingTask #38715

Conversation

Dr15Jones commented Jul 12, 2022

PR description:

PR validation:

cmsbuild commented Jul 12, 2022

cmsbuild commented Jul 12, 2022

Dr15Jones commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

Comparison Summary

makortel commented Jul 13, 2022

Dr15Jones commented Jul 13, 2022

Dr15Jones commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

makortel Jul 13, 2022

Choose a reason for hiding this comment

Dr15Jones Jul 13, 2022

Choose a reason for hiding this comment

makortel Jul 13, 2022

Choose a reason for hiding this comment

Dr15Jones Jul 13, 2022

Choose a reason for hiding this comment

makortel Jul 13, 2022

Choose a reason for hiding this comment

cmsbuild commented Jul 14, 2022

Comparison Summary

makortel commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

perrotta commented Jul 15, 2022