Delay constructing transition wrappers

[Dr15Jones]

PR description:

Do not create the edm::Run or edm::LuminosityBlock objects until they are requested.

PR validation:

The framework unit tests pass.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28459/12890

• This PR adds an extra 32KB to repository

[cmsbuild]

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

It involves the following packages:

FWCore/Framework

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

cms-bot commands are listed here

[Dr15Jones]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3602/console Started: 2019/11/22 23:17

[cmsbuild]

-1

Tested at: 2e1c1b4

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

• [HGCAL trigger] Mainly seeding and frontend Concentrator updates  #28385 @PFCal-dev: [HGCAL trigger] Mainly seeding and frontend Concentrator updates
• Fix alias in PreMixing configuration for APV simulation #28454 @fabiocos: Fix alias in PreMixing configuration for APV simulation
• No actual use for EcalClusterShapeFunction in HiEgammaAlgos #28428 @guitargeek: No actual use for EcalClusterShapeFunction in HiEgammaAlgos

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3602/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3602/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3602/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests RelVals

• Unit Tests:

I found errors in the following unit tests:

---> test testPhase2PixelNtuple had ERRORS

• RelVals:

When I ran the RelVals I found an error in the following workflows:
20034.0 step2

runTheMatrix-results/20034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D35+RecoFullGlobal_2026D35+HARVESTFullGlobal_2026D35/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D35+RecoFullGlobal_2026D35+HARVESTFullGlobal_2026D35.log

20434.0 step2

runTheMatrix-results/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D41+RecoFullGlobal_2026D41+HARVESTFullGlobal_2026D41/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D41+RecoFullGlobal_2026D41+HARVESTFullGlobal_2026D41.log

21234.0 step2

runTheMatrix-results/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D44_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D44+RecoFullGlobal_2026D44+HARVESTFullGlobal_2026D44/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D44_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D44+RecoFullGlobal_2026D44+HARVESTFullGlobal_2026D44.log

22034.0 step2

runTheMatrix-results/22034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D46_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D46+RecoFullGlobal_2026D46+HARVESTFullGlobal_2026D46/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D46_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D46+RecoFullGlobal_2026D46+HARVESTFullGlobal_2026D46.log

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

• [HGCAL trigger] Mainly seeding and frontend Concentrator updates  #28385 @PFCal-dev: [HGCAL trigger] Mainly seeding and frontend Concentrator updates
• Fix alias in PreMixing configuration for APV simulation #28454 @fabiocos: Fix alias in PreMixing configuration for APV simulation
• No actual use for EcalClusterShapeFunction in HiEgammaAlgos #28428 @guitargeek: No actual use for EcalClusterShapeFunction in HiEgammaAlgos

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3602/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3602/git-merge-result

[cmsbuild]

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

[Dr15Jones]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3606/console Started: 2019/11/24 15:37

[cmsbuild]

+1
Tested at: 2e1c1b4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3606/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81b28a/3606/summary.html

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 3 differences found in the comparisons
• DQMHistoTests: Total files compared: 34
• DQMHistoTests: Total histograms compared: 2785613
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2785270
• DQMHistoTests: Total skipped: 341
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
• Checked 147 log files, 16 edm output root files, 34 DQM output files

[Dr15Jones]

+1
The Reco difference is seen in other pull request.

[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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

[fabiocos]

+1

[slava77]

it looks like this PR added almost 10K issues in the static analysis report.
Should the mutables in the Event.h and LuminosityBlock.h be marked thread safe?
Combined they probably add up to most of the "mutable member if accessed via const pointer " kind of issues.

[slava77]

it looks like this PR added almost 10K issues in the static analysis report.
Should the mutables in the Event.h and LuminosityBlock.h be marked thread safe?
Combined they probably add up to most of the "mutable member if accessed via const pointer " kind of issues.

@Dr15Jones @smuzaffar @makortel please comment if marking the mutables in the Event.h and LuminosityBlock.h thread safe is OK.

[Dr15Jones]

@slava77 marking the change using CMS_THREAD_SAFE doesn't seem correct to me personally. That mutable is not safe to access across multiple threads, which is OK since edm::Event is itself only safe to use on one thread (each module actually get their own edm::Event instance).

So the proper thing would be able to mark edm::Event and edm::LuminosityBlock as being thread friendly but not const thread safe. At the moment, we do not have a way to designate that.

[slava77]

I see, is it practical to make these variables properly thread safe? (atomic or mutex)

[Dr15Jones]

I see, is it practical to make these variables properly thread safe? (atomic or mutex)

There are many mutable members inside edm::Event. Making them thread-safe would most likely cause performance issues and not give us any useful gain.

[Dr15Jones]

@slava77 there are two static analysis checks for mutables. One (the test that you looked at) just looks for mutable in any classes and the second looks for thread-unsafe mutables in classes we know get used across threads (e.g. Event or EventSetup data products). To avoid the problem you pointed out we could

1. add a new C++ attribute which denotes that the mutable has been reviewed and found not to be const thread safe. This would only be applied to the general mutable checker, not to the data products checker.
2. we could remove the general mutable checker.

So the question to you is, do you find the general mutable finder helpful or is it just noise?

[slava77]

So the question to you is, do you find the general mutable finder helpful or is it just noise?

they are somewhat useful

• in some cases the variables do not need to be mutable with slight changes in the code
• in some cases there is a reasonably cheap/simple fix by using std::atomic

The cases where a review was made and there will be no fix should not be counted/reported by default, assuming there is some annotation in the code to revisit these cases. This would be resolved by your case 1.

[Dr15Jones]

@slava77
Discussing the issue with Matti, we propose adding the attribute

[[cms::sa_allow]]

and the accompanying CPP define

CMS_SA_ALLOW

This can be used to tell the static analyzer (SA) to allow a construct it would previously have flagged as a problem. The policy would be that the use of the attribute also requires a comment stating why it should be allowed.

Matti and I feel this gives the most flexibility for the minimal change to the static analyzer as well as being easy to explain to developers.