Use modern C++ constructs in EventSetup #24270

Dr15Jones · 2018-08-12T13:23:22Z

This is a refactoring to improve the overall code quality of part of the EventSetup.

The name change matches the change to using std::move.

Use of an std::optional avoids additional calls to new/delete.

-switched to using from typedef -switched to constexpr from enum -use 'if constexpr' to handle recursive calls rather than use function overloading.

We know the length at compile time.

Variadic template arguments are more flexible than set number of arguments with dummy defaults.

The ability to use variadic templates with the edm::es::products function means it can now handle arbitrary numbers of arguments. The '<<' mechanism required lots of additional classes to function and is not as efficient as the new version of edm::es::products.

cmsbuild · 2018-08-12T13:23:39Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-08-12T13:26:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24270/5987

cmsbuild · 2018-08-12T13:26:24Z

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, @Martin-Grunewald, @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 · 2018-08-12T13:32:52Z

please test

cmsbuild · 2018-08-12T13:33:12Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29784/console Started: 2018/08/12 19:36

cmsbuild · 2018-08-12T22:14:25Z

+1
Tested at: 36db091
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24270/29784/summary.html

cmsbuild · 2018-08-12T22:14:28Z

Comparison job queued.

cmsbuild · 2018-08-12T23:43:33Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 28
DQMHistoTests: Total histograms compared: 2811739
DQMHistoTests: Total failures: 30
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2811528
DQMHistoTests: Total skipped: 181
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 27 files compared)
Checked 122 log files, 14 edm output root files, 28 DQM output files

cmsbuild · 2018-08-13T00:35:51Z

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 · 2018-08-13T08:11:30Z

FWCore/Framework/interface/Callback.h

@@ -47,12 +48,12 @@ namespace edm {
                             =CallbackSimpleDecorator<TRecord> >
      class Callback : public CallbackBase {
       public:
-         typedef TReturn (T ::* method_type)(const TRecord&);
+         using  method_type = TReturn (T ::*)(const TRecord&);


@Dr15Jones for my own education, is there a functional difference between these two syntaxes, or is it just aesthetics? Browsing around https://en.cppreference.com/w/cpp/language/type_alias I would say the latter, but I would like to know your motivation

The using directive can do everything the typedef can do and then even more. The modern C++ community seems to favor deprecating the use of typedef in favor of the more flexible using. That is why I've been converting code from using one to the other.

fabiocos · 2018-08-13T08:20:39Z

please test

@Dr15Jones do we expect changes because of this PR in the DQM output?

cmsbuild · 2018-08-13T08:23:02Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29792/console Started: 2018/08/13 10:25

cmsbuild · 2018-08-13T13:10:13Z

+1
Tested at: 36db091
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24270/29792/summary.html

cmsbuild · 2018-08-13T13:10:16Z

Comparison job queued.

Dr15Jones · 2018-08-13T13:40:55Z

@fabiocos asked

do we expect changes because of this PR in the DQM output?

This should not have changed DQM. However, my change to similar code last week also caused a series of DQM plots to be slightly different. We ultimately decided that was because those plots are not always reproducible. I'm assuming the same thing happened here.

Dr15Jones · 2018-08-13T13:57:32Z

One such difference, is 1 Tau more or less tau:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_10_3_X_2018-08-12-1100+24270/27959/10224.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU/L1T_L1TStage2CaloLayer2_NonIsolated-Tau.html

If this code change actually was the cause of a problem then I would have expected drastic problems (crashes, completely missing objects, etc) not something this subtle.

Dr15Jones · 2018-08-13T14:20:17Z

So I was looking in detail at the results from L1TStage2CaloLayer2. The histograms that are showing differences are booked in this directory:

cmssw/DQM/L1TMonitor/src/L1TStage2CaloLayer2.cc

Line 78 in 8af7aff

ibooker.setCurrentFolder(monitorDir_+"/NonIsolated-Tau");

and are filled within this if block:

cmssw/DQM/L1TMonitor/src/L1TStage2CaloLayer2.cc

Line 239 in 8af7aff

stage2CaloLayer2TauBxOcc_->Fill(itBX, itTau->hwPt());

The comparison tests say only TausPhi, TausEtEtaPhi and TausOcc differ by 1 entry, while TausEta and TausQual histograms are filled that the same time but do not show any difference in entries. I interpret this to mean that we do have the exact same number of Taus in the two jobs. Now the three histograms showing differences all use Phi in their binning. In we had a Phi that was shifted to be out of range of the histogram binning, that would explain the histograms not depending on Phi in the two job show the same entries while the ones use Phi differ by one.

I believe @slava77 has said that some of the L1 plots are known to have differences based on changes in underflow/overflow, so this is probably one of those cases.

cmsbuild · 2018-08-13T14:45:54Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 28
DQMHistoTests: Total histograms compared: 2811739
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2811556
DQMHistoTests: Total skipped: 181
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 27 files compared)
Checked 122 log files, 14 edm output root files, 28 DQM output files

fabiocos · 2018-08-13T19:52:06Z

+1

@Dr15Jones thank you for the check

Dr15Jones added 9 commits August 12, 2018 07:50

Allow operator= so move will work by default

7a8a4b5

Changed copy* and assign* function names to move*

293b5b2

The name change matches the change to using std::move.

Support std::optional as ESProducer return value

15e12fd

Use of an std::optional avoids additional calls to new/delete.

Removed unused functions

3318f5d

Use modern C++ constructs

e7c67e1

-switched to using from typedef -switched to constexpr from enum -use 'if constexpr' to handle recursive calls rather than use function overloading.

Use an std::array instead of a std::vector

e6adc28

We know the length at compile time.

Switched to using variadic template arguments

d26c5d0

Variadic template arguments are more flexible than set number of arguments with dummy defaults.

Directly fill the ESProducts from edm::es::products

3f2ff6f

cmsbuild added this to the CMSSW_10_3_X milestone Aug 12, 2018

cmsbuild added code-checks-pending comparison-pending core-pending orp-pending pending-signatures tests-pending labels Aug 12, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 12, 2018

cmsbuild added tests-started and removed tests-pending labels Aug 12, 2018

cmsbuild added tests-approved and removed tests-started labels Aug 12, 2018

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Aug 13, 2018

fabiocos reviewed Aug 13, 2018

View reviewed changes

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Aug 13, 2018

cmsbuild added tests-started and removed tests-pending labels Aug 13, 2018

cmsbuild added tests-approved and removed tests-started labels Aug 13, 2018

cmsbuild added comparison-available and removed comparison-pending labels Aug 13, 2018

Dr15Jones mentioned this pull request Aug 13, 2018

Irreproducible histogram entries in L1TStage2CaloLayer2 #24279

Open

cmsbuild added orp-approved and removed orp-pending labels Aug 13, 2018

cmsbuild merged commit 00d1548 into cms-sw:master Aug 13, 2018

Dr15Jones deleted the modernizeEventSetup branch August 31, 2018 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use modern C++ constructs in EventSetup #24270

Use modern C++ constructs in EventSetup #24270

Dr15Jones commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

Dr15Jones commented Aug 12, 2018

cmsbuild commented Aug 12, 2018 •

edited

Loading

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 13, 2018

fabiocos Aug 13, 2018

Dr15Jones Aug 13, 2018

fabiocos commented Aug 13, 2018

cmsbuild commented Aug 13, 2018 •

edited

Loading

cmsbuild commented Aug 13, 2018

cmsbuild commented Aug 13, 2018

Dr15Jones commented Aug 13, 2018

Dr15Jones commented Aug 13, 2018

Dr15Jones commented Aug 13, 2018

cmsbuild commented Aug 13, 2018

fabiocos commented Aug 13, 2018

Use modern C++ constructs in EventSetup #24270

Use modern C++ constructs in EventSetup #24270

Conversation

Dr15Jones commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

Dr15Jones commented Aug 12, 2018

cmsbuild commented Aug 12, 2018 • edited Loading

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 12, 2018

cmsbuild commented Aug 13, 2018

fabiocos Aug 13, 2018

Choose a reason for hiding this comment

Dr15Jones Aug 13, 2018

Choose a reason for hiding this comment

fabiocos commented Aug 13, 2018

cmsbuild commented Aug 13, 2018 • edited Loading

cmsbuild commented Aug 13, 2018

cmsbuild commented Aug 13, 2018

Dr15Jones commented Aug 13, 2018

Dr15Jones commented Aug 13, 2018

Dr15Jones commented Aug 13, 2018

cmsbuild commented Aug 13, 2018

fabiocos commented Aug 13, 2018

cmsbuild commented Aug 12, 2018 •

edited

Loading

cmsbuild commented Aug 13, 2018 •

edited

Loading