Change to global module in L1Trigger/L1TGEM

[Dr15Jones]

PR description:

• changed to global modules
• removed need to call Run transition
• reworked member functions to use 'return value optimization'

PR validation:

Code compiles.

[cmsbuild]

cms-bot internal usage

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43649/38308

• This PR adds an extra 16KB to repository

[cmsbuild]

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

It involves the following packages:

• L1Trigger/L1TGEM (l1)

@epalencia, @aloeliger, @cmsbuild can you please review it and eventually sign? Thanks.
@missirol, @JanFSchulte, @Martin-Grunewald this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

[Dr15Jones]

@makortel FYI

[Dr15Jones]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-72ad38/36708/summary.html
COMMIT: 5275e27
CMSSW: CMSSW_14_0_X_2024-01-04-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43649/36708/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 93 lines to the logs
• Reco comparison results: 16 differences found in the comparisons
• DQMHistoTests: Total files compared: 48
• DQMHistoTests: Total histograms compared: 3247277
• DQMHistoTests: Total failures: 1383
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3245872
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
• Checked 200 log files, 161 edm output root files, 48 DQM output files
• TriggerResults: no differences found

[aloeliger]

Not to slow everything down, but this seems a bit more in depth than the usual technical changes. If only for my edification could I get a quick explanation on what happened here?

[Dr15Jones]

If only for my edification could I get a quick explanation on what happened here?

Sometimes while doing these technical changes I come across some code that could use some 'more lovin' :). I had started by removing the member data that was just caching a EventSetup data product and moved the call to produce. This allowed beginRun to be removed. After that, I noticed that all the remaining member data were immutable so I could easily change the module to be a edm::global. (edm::global are preferred over edm::stream modules as only one instance of a edm::global is needed in a job while we must make a copy of each edm::stream per Stream in the job). After the edm::global change, I figured I could also use the new edm::Event::emplace syntax which avoids creating a temporary on the heap for the edm::Event::put call and instead use a stack temporary (less heap churn that way). I did that by changing the member function from having its results value be passed via mutable function argument (which is no longer considered a good solution by the C++ community at large) to have it be the return value of the function. (Modern C++ requires that the compiler now handle returning objects from function using an optimized mechanism).

[aloeliger]

If only for my edification could I get a quick explanation on what happened here?

Sometimes while doing these technical changes I come across some code that could use some 'more lovin' :). I had started by removing the member data that was just caching a EventSetup data product and moved the call to produce. This allowed beginRun to be removed. After that, I noticed that all the remaining member data were immutable so I could easily change the module to be a edm::global. (edm::global are preferred over edm::stream modules as only one instance of a edm::global is needed in a job while we must make a copy of each edm::stream per Stream in the job). After the edm::global change, I figured I could also use the new edm::Event::emplace syntax which avoids creating a temporary on the heap for the edm::Event::put call and instead use a stack temporary (less heap churn that way). I did that by changing the member function from having its results value be passed via mutable function argument (which is no longer considered a good solution by the C++ community at large) to have it be the return value of the function. (Modern C++ requires that the compiler now handle returning objects from function using an optimized mechanism).

Okay, thanks for the explanation, especially for what was happening to buildClusters and selectClusters which caught my eye in the first place.

[aloeliger]

+l1

[cmsbuild]

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. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

[rappoccio]

+1