Run G4 workers on their own dedicated threads #34899

Dr15Jones · 2021-08-16T19:48:01Z

PR description:

Each G4 worker now runs on its own thread. Each stream has its own thread and when the stream needs to do work it blocks the TBB thread and starts up its dedicated thread.

This should allow SimWatchers to be modified to call esConsumes.

PR validation:

Code compiles and a simple workflow runs.

OscarMTProducer now guarantees the class methods are run on their own dedicated thread.

Dr15Jones · 2021-08-16T19:48:54Z

@makortel FYI

cmsbuild · 2021-08-16T19:54:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34899/24707

This PR adds an extra 28KB to repository

cmsbuild · 2021-08-16T19:55:15Z

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

It involves the following packages:

SimG4Core/Application (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @cvuosalo, @rovere, @bsunanda, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2021-08-16T20:04:39Z

please test

cmsbuild · 2021-08-17T01:21:50Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-99abab/17811/summary.html
COMMIT: e058dfb
CMSSW: CMSSW_12_1_X_2021-08-16-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34899/17811/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: 55303 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 296373
DQMHistoTests: Total nulls: 238
DQMHistoTests: Total successes: 2703719
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -11.625 KiB( 38 files compared)
DQMHistoSizes: changed ( 10224.0 ): -0.117 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 250202.181 ): -0.410 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 25202.0 ): -0.543 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
DQMHistoSizes: changed ( 7.3 ): -9.066 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 8.0 ): -1.493 KiB SiStrip/MechanicalView
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: found differences in 11 / 38 workflows

civanch · 2021-08-17T09:38:44Z

@Dr15Jones , I would expect regression for this change or we change random seed?

Dr15Jones · 2021-08-17T13:38:49Z

@civanch

I would expect regression for this change or we change random seed?

Given the tests run single threaded, I would have expected exact comparisons. I'm making a change now where I properly propagate the random number seed to the new threads. Let's see if that avoids the differences.

civanch · 2021-08-17T13:47:31Z

@Dr15Jones , there is a simple method fully control random seeds. I would suggest to use it in the first place. For that, in OscarMTProducer uncomment line 217 and make line 216 and 217 edm::LogWarning.

cmsbuild · 2021-08-17T14:08:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34899/24723

This PR adds an extra 28KB to repository

cmsbuild · 2021-08-17T14:08:34Z

Pull request #34899 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

Dr15Jones · 2021-08-17T14:21:15Z

please test

cmsbuild · 2021-08-17T17:30:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-99abab/17829/summary.html
COMMIT: bb5bf39
CMSSW: CMSSW_12_1_X_2021-08-17-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34899/17829/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: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3000318
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

Dr15Jones · 2021-08-17T18:02:55Z

The DQM histogram differences are all from the MessageLogger and the reco differences are a known reproducibility problem caused by an uninitialized variable.
So the tests look good.

civanch · 2021-08-18T08:37:35Z

SimG4Core/Application/plugins/OscarMTProducer.cc

@@ -165,7 +177,13 @@ OscarMTProducer::OscarMTProducer(edm::ParameterSet const& p, const OscarMTMaster
  edm::LogVerbatim("SimG4CoreApplication") << "OscarMTProducer is constructed";
 }

-OscarMTProducer::~OscarMTProducer() {}
+OscarMTProducer::~OscarMTProducer() {
+  auto token = edm::ServiceRegistry::instance().presentToken();


@Dr15Jones , is it safe calling edm::ServiceRegistry::instance() in a destructor? May be better keeping the pointer as a class member?

The framework guarantees that it is safe to use the Service system in module destructors.

+1

If the framework guarantee the order of destruction, it is fine.

Dr15Jones · 2021-08-18T20:27:31Z

@civanch if you intended to approve the whole pull request (and not just the one comment) you have to do the + and 1 as a top level comment.

civanch · 2021-08-18T22:09:43Z

+1

cmsbuild · 2021-08-18T22:10:03Z

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 (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-08-19T05:57:22Z

+1

Dr15Jones added 5 commits August 16, 2021 12:17

Added ThreadHandoff class

7280fe5

Run G4 worker on its own dedicated thread

e51cf0d

applied code format

aa64686

Removed thread_local from RunManagerMTWorker

12c1f60

OscarMTProducer now guarantees the class methods are run on their own dedicated thread.

Be able to set worker thread stack size in configuration

e058dfb

cmsbuild added this to the CMSSW_12_1_X milestone Aug 16, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures simulation-pending tests-pending labels Aug 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 16, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 16, 2021

makortel mentioned this pull request Aug 16, 2021

Revamp threading model of OscarMTProducer cms-sw/framework-team#205

Closed

cmsbuild added tests-approved and removed tests-started labels Aug 17, 2021

Set random number system in worker thread

bb5bf39

cmsbuild added code-checks-pending and removed tests-approved code-checks-approved labels Aug 17, 2021

cmsbuild added the tests-pending label Aug 17, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 17, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 17, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 17, 2021

civanch reviewed Aug 18, 2021

View reviewed changes

cmsbuild added fully-signed simulation-approved and removed simulation-pending pending-signatures labels Aug 18, 2021

cmsbuild added orp-approved and removed orp-pending labels Aug 19, 2021

cmsbuild merged commit 6827de0 into cms-sw:master Aug 19, 2021

Dr15Jones deleted the omtWorkerThread branch August 24, 2021 18:27

makortel mentioned this pull request Aug 11, 2023

CMSEmStandardPhysicsEMMT, don't hold ref into main ParameterSet #42535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run G4 workers on their own dedicated threads #34899

Run G4 workers on their own dedicated threads #34899

Dr15Jones commented Aug 16, 2021

Dr15Jones commented Aug 16, 2021

cmsbuild commented Aug 16, 2021

cmsbuild commented Aug 16, 2021

Dr15Jones commented Aug 16, 2021

cmsbuild commented Aug 17, 2021

civanch commented Aug 17, 2021

Dr15Jones commented Aug 17, 2021

civanch commented Aug 17, 2021 •

edited

cmsbuild commented Aug 17, 2021

cmsbuild commented Aug 17, 2021

Dr15Jones commented Aug 17, 2021

cmsbuild commented Aug 17, 2021

Dr15Jones commented Aug 17, 2021

civanch Aug 18, 2021

Dr15Jones Aug 18, 2021

civanch Aug 18, 2021

Dr15Jones commented Aug 18, 2021

civanch commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

perrotta commented Aug 19, 2021

Run G4 workers on their own dedicated threads #34899

Run G4 workers on their own dedicated threads #34899

Conversation

Dr15Jones commented Aug 16, 2021

PR description:

PR validation:

Dr15Jones commented Aug 16, 2021

cmsbuild commented Aug 16, 2021

cmsbuild commented Aug 16, 2021

Dr15Jones commented Aug 16, 2021

cmsbuild commented Aug 17, 2021

Comparison Summary

civanch commented Aug 17, 2021

Dr15Jones commented Aug 17, 2021

civanch commented Aug 17, 2021 • edited

cmsbuild commented Aug 17, 2021

cmsbuild commented Aug 17, 2021

Dr15Jones commented Aug 17, 2021

cmsbuild commented Aug 17, 2021

Comparison Summary

Dr15Jones commented Aug 17, 2021

civanch Aug 18, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 18, 2021

Choose a reason for hiding this comment

civanch Aug 18, 2021

Choose a reason for hiding this comment

Dr15Jones commented Aug 18, 2021

civanch commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

perrotta commented Aug 19, 2021

civanch commented Aug 17, 2021 •

edited